Discarding new disposable on a disposed SerialDisposable
#688
Conversation
Discard a disposable on `SerialDisposable` when the instance of the `SerialDisposable` is already disposed.
DevAndArtist
changed the title
SerialDisposableSerialDisposable
| if isDisposed { | ||
| disposable?.dispose() | ||
| } else { | ||
| _inner.swap(disposable)?.dispose() |
There was a problem hiding this comment.
We need to check isDisposed again after swapping, since self could be disposed of in parallel during the process, racing against https://github.com/ReactiveCocoa/ReactiveSwift/pull/688/files#diff-7056bb5d45375e26f2eeb3adeabac16bR373.
There was a problem hiding this comment.
Can you provide a change suggestion and elaborate more why we need to do it, because I'm not sure I follow. I always thought that such operations should not be done in a multi-threading fashion, so it will be the users responsibility to serialize correctly.
If this PR gets approved, I'll add more documentations because this slightly changes the behavior of the type after it's disposed.
There was a problem hiding this comment.
Change suggestions do not seem to be available in this repo.
Essentially, we need to do this after the swapping in the else path:
if isDisposed {
_inner.swap(nil)?.dispose()
}
Because dispose() can be called in parallel to inner = new on different threads. Depending on the order observed by the system memory, it could yield a boundary case where isDisposed is true but the inner disposable is left untouched. The conflicting path may
I always thought that such operations should not be done in a multi-threading fashion, so it will be the users responsibility to serialize correctly.
Disposables have always meant to be thread safe, despite us not explicitly having written this as part of the contract.
There was a problem hiding this comment.
Change suggestions do not seem to be available in this repo
Weird. I don't see a setting for it.
Can you elaborate on why you think this is the case? I shared some thoughts on #686, but I don't see why this is necessarily better or more correct. |
|
@mdiep This PR doesn’t change the semantic of I would consider #686 a bug in the sense that the inner disposable has been disposed of, but the storage still retains a strong reference to the disposed inner.
Allowing the inner to be retained is also inconsistent with |
This is an experimental PR to run tests against this change.
Please DO NOT MERGE iff this change goes against the design.Update: @mdiep all tests passed. Next question is if this a reasonable fix or not. For more information please read the linked issue. Other then that I'd appreciate if someone would guide me from now on.
Checklist