Skip to content

Make page load async when using endpoint routing #7938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Mar 4, 2019
Merged

Make page load async when using endpoint routing #7938

merged 4 commits into from
Mar 4, 2019

Conversation

pranavkm
Copy link
Contributor

@pranavkm pranavkm commented Feb 26, 2019 •
edited
Loading

Fixes #8016

@pranavkm pranavkm requested review from JamesNK and rynowak February 26, 2019 02:42
pranavkm
pranavkm commented Feb 26, 2019
View reviewed changes
@rynowak
Copy link
Member

rynowak commented Feb 26, 2019

This is D O P E

rynowak
rynowak reviewed Feb 26, 2019
View reviewed changes
rynowak
rynowak reviewed Feb 26, 2019
View reviewed changes
rynowak
rynowak reviewed Feb 26, 2019
View reviewed changes
rynowak
rynowak reviewed Feb 26, 2019
View reviewed changes
JamesNK
JamesNK reviewed Feb 26, 2019
View reviewed changes
Copy link
Member

@JamesNK JamesNK left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What problem is changing the type to be async solving? In some places it is moving GetResult from one place to another.

rynowak
rynowak reviewed Feb 26, 2019
View reviewed changes
mkArtak
mkArtak reviewed Feb 26, 2019
View reviewed changes
@mkArtak
Copy link
Contributor

mkArtak commented Feb 26, 2019

Can you please link the issue this addresses?

mkArtak
mkArtak reviewed Feb 26, 2019
View reviewed changes
@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Feb 26, 2019
@pranavkm pranavkm added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Feb 26, 2019
@pranavkm
Copy link
Contributor Author

@JamesNK filed an issue for this - #8016. But in essence we make a blocking call when loading the page, which performs async work. We end up getting reports of application deadlocks due to thread pool starvation. This change attempts to address it fairly cheaply with endpoint routing.

pranavkm
pranavkm commented Feb 28, 2019
View reviewed changes
rynowak
rynowak reviewed Mar 1, 2019
View reviewed changes
src/Mvc/Mvc.RazorPages/src/Infrastructure/CompiledPageActionDescriptorCache.cs Outdated
}
}

public virtual ValueTask<CompiledPageActionDescriptor> GetOrAddAsync(PageActionDescriptor actionDescriptor)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really a good usage of ValueTask? I don't think ValueTask can be awaited multiple times. Also the usual pattern for this is to cache Task<T> instead of caching T

@rynowak
Copy link
Member

rynowak commented Mar 1, 2019

Does this really need the breaking change label? Is the breaking change that we're not calling an old API anymore?

@pranavkm pranavkm force-pushed the prkrishn/async-load branch 2 times, most recently from a34f5ee to eb9c04c Compare March 2, 2019 00:19
pranavkm added 3 commits March 2, 2019 07:52
@pranavkm
Make page load async
79e6d0e
@pranavkm
Changes per PR comments
7fee202 
* Use ActionDescriptorCollection.Version to invalidate PageLoader cache
* Add tests, changes per PR
@pranavkm
Update ref assemblies
919eb51
@pranavkm pranavkm removed the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Mar 2, 2019
@pranavkm pranavkm force-pushed the prkrishn/async-load branch from eb9c04c to 919eb51 Compare March 2, 2019 15:57
@pranavkm
Copy link
Contributor Author

pranavkm commented Mar 2, 2019

Does this really need the breaking change label? Is the breaking change that we're not calling an old API anymore?

Not any longer. I wasn't registering the IPageLoader in one of the iterations which could have been construed as breaking. All we have now is the ObsoleteAttribute on IPageLoader

@pranavkm
Copy link
Contributor Author

pranavkm commented Mar 2, 2019

🆙 📅

rynowak
rynowak reviewed Mar 2, 2019
View reviewed changes
src/Mvc/Mvc.RazorPages/src/Infrastructure/PageLoader.cs
public abstract class PageLoader : IPageLoader
#pragma warning restore CS0618 // Type or member is obsolete
{
public abstract Task<CompiledPageActionDescriptor> LoadAsync(PageActionDescriptor actionDescriptor);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DOCS YO

rynowak
rynowak approved these changes Mar 2, 2019
View reviewed changes
Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@pranavkm pranavkm merged commit 39e5257 into master Mar 4, 2019
@pranavkm pranavkm deleted the prkrishn/async-load branch March 4, 2019 22:46
@pranavkm pranavkm mentioned this pull request Mar 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesNK JamesNK JamesNK left review comments

@javiercn javiercn javiercn left review comments

@mkArtak mkArtak mkArtak left review comments

@rynowak rynowak rynowak approved these changes

Assignees
No one assigned
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pranavkm @rynowak @mkArtak @JamesNK @javiercn @Eilon