Skip to content
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.

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

Support C# Async Constructs During Page Rendering #3984

Merged
merged 3 commits into from
Oct 6, 2020

Conversation

bdukes
Copy link
Contributor

@bdukes bdukes commented Aug 14, 2020

Summary

Thanks @mitchelsellers for tracking down the answer to our async issues.

This turns on Async for Default.aspx.

Previously, when we attempted this in #2089, we ran into an issue with the culture not being preserved throughout the entire page lifecycle. So, for example, the page may have been loaded in a non-English context, but the URLs were generated for en-US anyway.

The solution is to set the thread culture via an IHttpModule implementation, rather than via the page's OnInit method. This allows the culture to persist throughout the page request.

This is in preparation to move the Localization.SetThreadCultures call
to an HttpModule.
This is in support of turning on Async for Default.aspx. Per a
discussion with the ASP.NET team, setting the thread culture in OnInit
for a page does not persist in the context for the page (once Async is
turned on an there are multiple contexts to consider). Setting it from
an IHttpModule implementation, however, will persist throughout that
request.

This fixes the bug that plagued dnnsoftware#2089, where, as an example, URLs would
be generated for the incorrect language once Async was on.
@bdukes bdukes added this to the 9.8.0 milestone Aug 14, 2020
@david-poindexter
Copy link
Contributor

This is fantastic!

@mitchelsellers
Copy link
Contributor

This is awesome. I’ll review over the weekend

@valadas
Copy link
Contributor

valadas commented Aug 15, 2020

Whohoo

Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

This implementation matches the recommendation from Microsoft, initial testing is looking good as well.

When we get this out I want to call this out for specific testing from international users, but approved

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@donker donker merged commit a2f73ac into dnnsoftware:develop Oct 6, 2020
@bdukes bdukes deleted the webforms-async branch November 30, 2020 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants