-
Notifications
You must be signed in to change notification settings - Fork 401
Add new SetAction overload to avoid fire & forget silent error #2565
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
Conversation
hide it from intellisense, so the users are more likely to use the overload that takes CancellationToken
/// </remarks> | ||
// Hide from intellisense, it's public to avoid the compiler choosing a sync overload | ||
// for an async action (and fire and forget issue described in https://github.com/dotnet/command-line-api/issues/2562). | ||
[EditorBrowsable(EditorBrowsableState.Never)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you wanted to force folks to think about the CancellationToken
you could also mark it obsolete -- not sure how hardcore you are about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my first idea, but then I realized that we should most likely not release new public [Obsolete]
methods. But I agree, it would be even better as it would allow us emit a compiler warning/error with a clear message.
@jonsequitur thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to differentiate the methods by name rather than using overloads, e.g. SetAction
and SetAsyncAction
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to differentiate the methods by name rather than using overloads, e.g.
SetAction
andSetAsyncAction
?
I like the SetAsyncAction
name idea (we never discussed actions at the API review), but users could still run into this particular bug and it would cause another wave of breaking changes in SDK/VMR etc. Let's discuss it offline, I think it's now or never.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should most likely not release new public [Obsolete] methods
dotnet/Open-XML-SDK#1600 added ObsoleteAttribute to several new types and methods. There though, they intend to remove the attribute eventually, unlike here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this 🙏
I feel this deserves a more general Roslyn analyzer, which would warn whenever an async lambda expression as an argument of a call is implicitly converted to a void-returning delegate type. If the developer really wants an async void function, they can replace the lambda expression with a named local function so that the The analyzer could abstain from warning if there is no overload that would accept some sort of task-type-returning delegate instead. However, I suspect there wouldn't be too many warnings even without this check. |
It's great idea, would you like to open an issue in the analyzers repo? |
I found some pre-existing feature requests for warning about async void:
At this time then, it does not seem useful to file a new request there. But I'll be looking into whether I can start using the vs-threading analysers. |
fixes #2562
cc @ericstj