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

Allow default AsyncFlowControls rather than throwing #82912

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

stephentoub
Copy link
Member

ExecutionContext.SuppressFlow currently throws an exception if flow is already suppressed. This makes it complicated to use, as you need to check whether IsFlowSuppressed first and take two different paths based on the result. If we instead just allow SuppressFlow to return a default AsyncFlowControl rather than throwing, and have AsyncFlowControl's Undo nop rather than throw if it doesn't contain a Thread, we can again make it simple to just always use SuppressFlow without any of the other complications.

@kouvel, thoughts on doing this? On the one hand, the current exceptions can prevent some misuse. On the other, it makes using the type much harder. On balance I think it's better to make a change like that in this PR, but I'd like your insights.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Mar 2, 2023

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

ExecutionContext.SuppressFlow currently throws an exception if flow is already suppressed. This makes it complicated to use, as you need to check whether IsFlowSuppressed first and take two different paths based on the result. If we instead just allow SuppressFlow to return a default AsyncFlowControl rather than throwing, and have AsyncFlowControl's Undo nop rather than throw if it doesn't contain a Thread, we can again make it simple to just always use SuppressFlow without any of the other complications.

@kouvel, thoughts on doing this? On the one hand, the current exceptions can prevent some misuse. On the other, it makes using the type much harder. On balance I think it's better to make a change like that in this PR, but I'd like your insights.

Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Threading

Milestone: -

@stephentoub stephentoub added this to the 8.0.0 milestone Mar 2, 2023
ExecutionContext.SuppressFlow currently throws an exception if flow is already suppressed.  This makes it complicated to use, as you need to check whether IsFlowSuppressed first and take two different paths based on the result.  If we instead just allow SuppressFlow to return a default AsyncFlowControl rather than throwing, and have AsyncFlowControl's Undo nop rather than throw if it doesn't contain a Thread, we can again make it simple to just always use SuppressFlow without any of the other complications.
@kouvel
Copy link
Member

kouvel commented Mar 3, 2023

Seems reasonable to me. I'm not sure how useful the SuppressFlow exception is, RestoreFlow would throws when trying to restore a flow-unsuppressed context, maybe that's sufficient.

kouvel
kouvel approved these changes Mar 3, 2023
@stephentoub
Copy link
Member Author

Cool, thanks.

@ericsampson
Copy link

Nice!

Will the API docs be updated automatically to show that these exceptions are now longer a possible outcome?

Is the change going to be mentioned in general docs anywhere (or do existing mentions need to be updated) ?

Thanks for this very nice little QoL improvement <3

@stephentoub
Copy link
Member Author

Will the API docs be updated automatically to show that these exceptions are now longer a possible outcome?

Not automatically. We'll need to do so manually once the change hits official preview bits, which should be Preview 3 (since that's what main is currently targeting).

Is the change going to be mentioned in general docs anywhere (or do existing mentions need to be updated) ?

Anywhere the docs currently talk about or show needing to check IsFlowSuppressed we'll want to update with a clarification about pre-.NET 8.

Thanks for this very nice little QoL improvement <3

You're welcome.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants