feat: add support for CancellationToken in publishers #202
Conversation
igorgiovannini
force-pushed
the
feature/cancellation_token
branch
from
64d82b5
to
fa54ac9
Compare
igorgiovannini
force-pushed
the
feature/cancellation_token
branch
from
fa54ac9
to
ea82555
Compare
| /// <returns> | ||
| /// The <see cref="ISilverbackBuilder" /> so that additional calls can be chained. | ||
| /// </returns> | ||
| public static ISilverbackBuilder AddDelegateSubscriber<TMessage>( |
| @@ -251,6 +251,9 @@ public override void Dispose() | |||
| { | |||
| foreach (var lazyStream in _lazyStreams) | |||
| { | |||
| if (cancellationToken.IsCancellationRequested) | |||
There was a problem hiding this comment.
It's probably more correct to throw in this case.
Usually, a cancellable method is supposed to throw the OperationCancelledException when it aborts. This allows the caller to know whether it has actually completed before the cancellation occurred or it had to cancel. (There's a practical cancellationToken.ThrowIfCancellationRequested().)
I didn't carefully check the implications of a throw in here though.
| /// <returns> | ||
| /// A <see cref="Task" /> representing the asynchronous operation. | ||
| /// </returns> | ||
| Task PublishAsync(object message); | ||
| Task PublishAsync(object message, CancellationToken cancellationToken = default); |
There was a problem hiding this comment.
Adding an optional parameter unfortunately introduces a subtle breaking change.
The overload to be invoked is resolved at compile time and at that moment the compiler adds the default values as well. Therefore if you compiled with a version that didn't declare the optional parameter but you run against the later version which does, you will still run into the MissingMethodException.
Let's imagine to have a common library that references the current version of Silverback and calls one of these methods. This library is published as nuget and referenced in a .net application. If the application would upgrade to the new version of Silverback with the optional parameter, it would start to throw.
I tried it, to be absolutely sure: Unhandled exception. System.MissingMethodException: Method not found: 'System.Threading.Tasks.Task Silverback.Messaging.Publishing.IPublisher.PublishAsync(System.Object)'.
It would be sufficient to recompile the common library, that's true, but it's still a breaking change that I'd prefer to avoid.
The solution is simple (but a bit annoying) and is to add an overload with the extra parameter, instead of using an optional one.
@igorgiovannini if you don't have time or you're not up for it, I will take care of it. 😃
| { | ||
| Check.NotNull(context, nameof(context)); | ||
| Check.NotNull(next, nameof(next)); | ||
|
|
||
| foreach (var enricher in context.Envelope.Endpoint.MessageEnrichers) | ||
| { | ||
| enricher.Enrich(context.Envelope); | ||
|
|
||
| if (cancellationToken.IsCancellationRequested) |
There was a problem hiding this comment.
Same as mentioned already, we maybe should throw.
| .ConfigureAwait(false); | ||
|
|
||
| if (stoppingToken.IsCancellationRequested) | ||
| if (cancellationToken.IsCancellationRequested) |
There was a problem hiding this comment.
Same, we should probably throw and handle (log?) if needed.
| @@ -47,18 +51,21 @@ public async Task HandleAsync(ProducerPipelineContext context, ProducerBehaviorH | |||
| if (!sequenceWriter.CanHandle(context.Envelope)) | |||
| continue; | |||
|
|
|||
| if (cancellationToken.IsCancellationRequested) | |||
There was a problem hiding this comment.
Throw instead of silently aborting here too?
This PR adds support for CancellationToken in publishers (and properly propagates it through the chain) as described in issue #116