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

Remove WithCurrentCulture (Resolves #862) #2108

Merged
merged 1 commit into from May 4, 2015
Merged

Remove WithCurrentCulture (Resolves #862) #2108

merged 1 commit into from May 4, 2015

Conversation

bricelam
Copy link
Contributor

@bricelam bricelam commented May 1, 2015

No description provided.

@ghost ghost added the cla-already-signed label May 1, 2015
@divega
Copy link
Contributor

divega commented May 1, 2015

Don't we still need to chain ConfigureAwait(false) to each async call to avoid dealocks in UI apps?

If that was the case, I would suggest we went with a different approach which is to "#ifdef" the unnecessary code from WithCurrentCulture() out on specific platforms. We could then talk about renaming WithCurrentCulture() to something else, e.g. MakeThisAsyncCallWork() 😄

@bricelam
Copy link
Contributor Author

bricelam commented May 1, 2015

Ah, I missed that part. 😄 This was just a desperate effort to try and get the tests passing on my machine. It didn't work. I'll leave the work for later since it's marked as pri1.

@bricelam bricelam closed this May 1, 2015
@bricelam
Copy link
Contributor Author

bricelam commented May 1, 2015

Re-openeing. Confirmed with @AndriySvyryd that ConfigureAwait isn't really needed on modern platforms.

@bricelam bricelam reopened this May 1, 2015
@ghost ghost added the cla-already-signed label May 1, 2015
@bricelam
Copy link
Contributor Author

bricelam commented May 1, 2015

P.S. If we do end up bringing this back. I propose we call it .MakeItSo()

@divega
Copy link
Contributor

divega commented May 1, 2015

👍 at .MakeItSo() 😄

@AndriySvyryd
Copy link
Member

:shipit:

@StephenCleary
Copy link

Confirmed with @AndriySvyryd that ConfigureAwait isn't really needed on modern platforms.

Not sure what this means. ConfigureAwait is beneficial in all UI contexts, including modern ones.

@AndriySvyryd
Copy link
Member

@StephenCleary This is referring to modern platforms having async entry points/callbacks. And that as long as async methods are only invoked from other async methods ConfigureAwait(false) is not needed to prevent deadlocks.

@StephenCleary
Copy link

That's only one purpose of ConfigureAwait(false).

ConfigureAwait(false) documents the code as not needing a context. Also, it is more efficient (when a context is present) - in particular, it prevents performance death by a thousand paper cuts.

And there are still scenarios where sync-over-async is necessary, and there likely will always be (think third-party library extension points / callbacks that are synchronous). ConfigureAwait(false) is still necessary to prevent deadlock in those scenarios.

@AndriySvyryd
Copy link
Member

AndriySvyryd commented Mar 31, 2017

@StephenCleary We handle those scenarios by offering a sync version of every method. Sync is faster than async even with ConfigureAwait(false) and it doesn't have any deadlock potential.

ConfigureAwait(false) litters the code, it's easy to forget to use it and could cause nonobvious issues where a series of library calls ends up invoking user code that does need context.

@StephenCleary
Copy link

Yes, the synchronous versions handle the synchronous scenarios.

There's still the "paper cuts" issue - by that I mean that the general guidance is to not have more than a hundred continuations per second on a UI thread. This can be reached pretty quickly if there's deeply-nested async calls all of which resume on the context.

But it's pretty rare, and probably not worth the clutter that ConfigureAwait(false) would cause; if a user does run into that performance issue, they can call the asynchronous EF APIs outside of a UI context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants