ICompletableSynchronizedStorageSession now inherits from IAsyncDisposable
#7329
Conversation
…posable` instead of `IDisposable`
|
After a discussion with @danielmarbach it became clear that |
…ompletableSynchronizedStorageSession` and revert to also implement `IDisposable` to make this a non-breaking change.
| @@ -85,7 +85,7 @@ public MyTask(Context scenarioContext, IServiceProvider provider) | |||
| protected override async Task OnStart(IMessageSession session, CancellationToken cancellationToken = default) | |||
| { | |||
| using (var scope = provider.CreateScope()) | |||
There was a problem hiding this comment.
I'm wondering if we should mimic the MainPipelineExecutor's behavior here and use an AsyncScope in these situations.
There was a problem hiding this comment.
I originally added this test to verify that usage outside the pipeline works, to make sure transactional session usage will never be broken. I think it would be reasonable to argue we should switch to AsyncScope here, since the core favors that one. Although from a down stream integration perspective, one could argue both scopes should work as long as we support the sync and async variants. Is it reasonable to consider in this test to test both paths?
There was a problem hiding this comment.
IMHO that is testing infrastructure. We must assume .net will correctly invoke DisposeAsync and Dispose.
If its important to test Dispose behavior itself wouldn't it be better to have unittests for the specific types?
There was a problem hiding this comment.
That's a fair angle. AsyncScope only then 🪂
...us.PersistenceTests/Sagas/When_worker_tries_to_complete_saga_update_by_another_optimistic.cs
Show resolved
Hide resolved
| @@ -85,7 +85,7 @@ public MyTask(Context scenarioContext, IServiceProvider provider) | |||
| protected override async Task OnStart(IMessageSession session, CancellationToken cancellationToken = default) | |||
| { | |||
| using (var scope = provider.CreateScope()) | |||
There was a problem hiding this comment.
I originally added this test to verify that usage outside the pipeline works, to make sure transactional session usage will never be broken. I think it would be reasonable to argue we should switch to AsyncScope here, since the core favors that one. Although from a down stream integration perspective, one could argue both scopes should work as long as we support the sync and async variants. Is it reasonable to consider in this test to test both paths?
...cceptanceTesting/AcceptanceTestingPersistence/AcceptanceTestingSynchronizedStorageSession.cs
Outdated
Show resolved
Hide resolved
src/NServiceBus.Core/Pipeline/Incoming/LoadHandlersConnector.cs
Outdated
Show resolved
Hide resolved
src/NServiceBus.Core/Pipeline/Incoming/LoadHandlersConnector.cs
Outdated
Show resolved
Hide resolved
...us.PersistenceTests/Sagas/When_worker_tries_to_complete_saga_update_by_another_optimistic.cs
Show resolved
Hide resolved
ramonsmits
deleted the
ICompletableSynchronizedStorageSession-IAsyncDisposable
branch
| @@ -30,5 +30,7 @@ public void Dispose() | |||
| { | |||
| } | |||
|
|
|||
| public ValueTask DisposeAsync() => default; | |||
ICompletableSynchronizedStorageSessionnow inherits fromIAsyncDisposableinstead ofIDisposableAlternatively
ICompletableSynchronizedStorageSessionshould not inherit either fromIDisposableorIAsyncDisposableas this usually is deferred to the implementation.However, that causes issue with this specific interfaces as Dispose is used for transaction rollback.