ReactiveSwift icon indicating copy to clipboard operation
ReactiveSwift copied to clipboard

Discarding new disposable on a disposed `SerialDisposable`

Open DevAndArtist opened this issue 7 years ago • 2 comments

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

  • [ ] Updated CHANGELOG.md.

DevAndArtist avatar Oct 27 '18 17:10 DevAndArtist

Semantically it is the right fix

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 avatar Dec 18 '18 20:12 mdiep

@mdiep This PR doesn’t change the semantic of SerialDisposable disposing of any inner disposable set after itself had been disposed 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.

CompositeDisposable does not hold reference to any inner beyond its disposal. ScopedDisposable does, but its nature is different from these two mutable disposables.

Allowing the inner to be retained is also inconsistent with dispose(), which clears the inner disposable. I think it makes sense to say inner == nil && isDisposed should always hold.

andersio avatar Dec 19 '18 10:12 andersio