Add a JTF-derived type that cancels a DisposalToken and drains the JoinableTaskCollection when disposed#1539
Conversation
… called. - Add an internal event to JoinableTaskCollection which is raised when JoinTillEmptyAsync is called - Add SelfCancellingJoinableTaskFactory, a variant of DelegatingJoinableTaskFactory which contains a CancellationTokenSource and cancels it during shutdown (signalled by that event, or Dispose being called)
…gn for a cancelable JTF
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
AArnott
changed the title
lifengl
left a comment
lifengl
left a comment
There was a problem hiding this comment.
While this could be useful utility wrap within some kinds of usage, but I feel making a special kind of JoinableTaskFactory disposable doesn't fit the overall design pattern very well. The Disposed factory is actually still functional as other caller can still call RunAsync to start a task, with or without taking care of the CancellationToken, and injecting another task to the underline collection.
Overall, a JTF factory may never die, even when all JTF Task spinning off from it are all completed, but any of the child spin off JTF task from them might still be alive, and they would all be backed chained to the JoinableTaskFactory and potentially call its method, so making one disposable is somewhat odd from that logic point of view.
| { | ||
| if (!this.disposalTokenSource.IsCancellationRequested) | ||
| { | ||
| this.disposalTokenSource.Cancel(); |
There was a problem hiding this comment.
This code is not thread-safe. When Dispose is called on two threads at the same time, one might run into ObjectDisposedException. The definition of the disposable pattern requires Dispose to be thread-safe and can be called any number of times.
There was a problem hiding this comment.
Not many classes are thread-safe for disposal.
I disagree that the disposable pattern requires thread-safety. It requires that Dispose can be called multiple times, but it doesn't specify that this may happen concurrently.
I agree we could improve this though.
| public DisposableJoinableTaskFactory(JoinableTaskFactory innerFactory) | ||
| : base(innerFactory) | ||
| { | ||
| Requires.Argument(this.Collection is not null, nameof(innerFactory), "A collection must be associated with the factory."); |
There was a problem hiding this comment.
The design is a little odd that this factory wraps an inner factory, and when it is disposed, it waits a JoinableTaskCollection potentially used by others
There was a problem hiding this comment.
it waits a JoinableTaskCollection potentially used by others
I'm not sure I agree with that. You can't get a collection out of a factory, so unless the factory creator chose to create and share a collection, no one else could be adding to that collection.
Of course disposal causing a block for pending tasks is odd for this library, but blocking on tasks on disposal isn't at all odd for VS.
|
Personally, I will question what is the mis-use cases we talked about. I don't feel generally we have a cancellation token - JoinableTaskCollection alignments. That is specific scenario which things are being misused is mostly inside VS package, where tearing down a package often ends by waiting a JTF collection, and it should use a cancellation token which is being triggered when the package need to be closed. It might be more a problem of the VS package pattern than a pattern to be forced in the JTF level. |
|
Thank you for your review, @lifengl. I share your concern about this being a good fit for the library (even after my changes). |
3 participants
The motivation behind this change is that I work in a codebase that has many misuses of JTF, where a cancellation token which is not associated with the lifetime of that JTF is used. I would like to have access to a token that is guaranteed to have been cancelled during JoinTillEmptyAsync, and I think putting that token inside of the JTF will make it easier to pass along and update existing code to use the correct token.
One issue I see with this idea is that call sites that want to make use of the DisposalToken will need to cast or pattern match on the SelfCancellingJoinableTaskFactory type. Would it be appropriate to also add an extension method that handles that?