Use the helper methods for IDisposable-fields, avoid repeating code patterns #556
Conversation
| @@ -28,7 +28,7 @@ public ContextDisposable(SynchronizationContext context, IDisposable disposable) | |||
| throw new ArgumentNullException(nameof(disposable)); | |||
|
|
|||
| Context = context; | |||
| _disposable = disposable; | |||
| Disposable.TrySetSingle(ref _disposable, disposable); | |||
There was a problem hiding this comment.
Is TrySetSingle required here?
There was a problem hiding this comment.
This is in the constructor so nothing else should be able to call ContextDisposable.Dispose() at this point.
| @@ -28,7 +28,7 @@ public ScheduledDisposable(IScheduler scheduler, IDisposable disposable) | |||
| throw new ArgumentNullException(nameof(disposable)); | |||
|
|
|||
| Scheduler = scheduler; | |||
| _disposable = disposable; | |||
| Disposables.Disposable.TrySetSingle(ref _disposable, disposable); | |||
There was a problem hiding this comment.
Is TrySetSingle required here?
There was a problem hiding this comment.
Same as above, this is in the constructor where it can't be disposed yet.
| Drain(); | ||
| } | ||
| } | ||
| if (Disposable.TrySetSerial(ref currentSubscription, null)) |
There was a problem hiding this comment.
Does this still capture the original semantics?
There was a problem hiding this comment.
There is a loop in TrySetSerial where a concurrent dispose failing the CAS will have it repeat the loop, detect that it needs to dispose d and then quits. This logic relies on the fact that if the single CAS fails, it can be only due to cancellation and there is nothing to do further.
|
@akarnokd I added some notes for further review, thx. |
| @@ -131,7 +129,8 @@ void Drain() | |||
| else | |||
| { | |||
| var sad = new SingleAssignmentDisposable(); | |||
| if (Interlocked.CompareExchange(ref currentSubscription, sad, null) == null) | |||
|
|
|||
| if (Disposable.TrySetSingle(ref currentSubscription, sad) == TrySetSingleResult.Success) | |||
There was a problem hiding this comment.
@akarnokd This change was added (the original reason to split SetSingle and TrySetSingle).
No description provided.