4.x: Inline SerialDisposable usages #593
Conversation
| _subscription.Disposable = d; | ||
| d.Disposable = result.SubscribeSafe(new HandlerObserver(this)); | ||
| _once = true; | ||
| Disposable.TrySetSerial(ref _subscription, result.SubscribeSafe(this)); |
There was a problem hiding this comment.
I might not ever get threading right and be stuck with using lock forever, but aren't we introducing a race here, since the subscription might invoke OnError on a different thread almost immediately, and _once is not protected by anything, it's not a volatile write and there's no locks.
There was a problem hiding this comment.
Whenever this is handed to another thread, there is a barrier inside the scheduler or task start that establishes a happens-before, so setting _once to true should be always visible to the next call to OnError.
There was a problem hiding this comment.
There was a problem hiding this comment.
Where is the specific part about this being passed to another thread?
There was a problem hiding this comment.
It's implicit as it is just another value/variable: See the "Full memory barrier" section about what constitutes as barrier and thus anything set before that will be visible after (provided that no further changes happen on the original thread to those references).
There was a problem hiding this comment.
But we don't know nothing about how the work will be unloaded to another thread, threre might not be a new thread or a Task.Start. It might just go onto an existing EventLoopScheduler and be picked up from there.
There was a problem hiding this comment.
Such async boundaries will go through eventually a lock or an atomic CAS/Exchange which are full barriers. If they didn't, everything else would break way before this field is ever read.
There was a problem hiding this comment.
Please, just make it a Volatile.Write for now and push that optimization to a separate PR. I'm really not convinced but my experience with lock-free programming is also limited. Thus I can't merge it as it is in good faith.
This PR inlines the usage of the
SerialDisposableclass into a field andDisposable.TrySetSerialcalls.