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

Async event handlers #571

Closed

Conversation

sdijoseph
Copy link
Contributor

I had a use case for the UpdateDetected event where I wanted to call asynchronous code inside the event handler. With the current code, it doesn't seem possible to call async code within the handler and actually ensure that code finishes executing before SparkleUpdater continues it's normal flow.

I'm proposing in this PR to make SparkleUpdater's events AsyncEventHandlers, so the completion of all handler for each event can be awaited inside the SparkleUpdater code using InvokeAsync. I'm using the implementation provided by the Microsoft.VisualStudio.Threading package. (This package adds a ton of warnings related to async code, so I set the NoWarn flag for them)

This is a breaking change, as current handlers will now have to return "Task.CompletedTask" since the handlers are expected to return Task instead of void. I'm submitting this as a draft PR with only UpdateDetected converted to AsyncEventHandler. If you agree with this approach, I can convert the remaining events as well.

Stephen DiJoseph added 2 commits March 19, 2024 10:03
Add
Microsoft.VisualStudio.Threading package.

- Use their implementation of AsyncEventHandler.
@Deadpikle
Copy link
Collaborator

Hey, thank you for sending in this PR. I'm holding off on it for now until I figure out what to do about the various threading issues in this lib, since they might require more extensive changes. I really appreciate it :-) Not closing for now until I have time to look at it more closely. Thanks!

@Deadpikle
Copy link
Collaborator

Hi @sdijoseph,

Thank you again for making this contribution and doing the research, work, etc. :-) I really appreciate the time and effort you put in.

After taking a look at things and doing some work in #609, I've decided not to merge this PR. Instead, I added a UpdateDetectedAsync event that can be used for this use case to keep things simple for end-users and not adding another dependency to NetSparkleUpdater. If other events need to be async, we can add async-style, Task returning versions of those events rather than changing the already-existing events to use AsyncEventHandlers.

The other option was to use a callback that the user needed to call whenever they were done doing work but again, to keep things simple, I just added an async version of the existing event.

I did take a look at other events and didn't immediately see ones that needed to be handled async in obvious ways, so I'll wait for users to let me know which ones they are needing async versions of.

So, I'll be closing this PR and not merging it. Again, thank you for your time and for making me aware of the need for an async event, here!

@Deadpikle Deadpikle closed this Aug 16, 2024
Deadpikle added a commit that referenced this pull request Aug 16, 2024
@sdijoseph
Copy link
Contributor Author

@Deadpikle Great! When are you planning to release another pre-release package on NuGet?

@Deadpikle
Copy link
Collaborator

@sdijoseph I'm working on the last planned major-breaking-changes PR in #614. So probably after that or thereabouts.

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.

2 participants