fsprojects / FSharp.Control.Reactive

Extensions and wrappers for using Reactive Extensions (Rx) with F#.
http://fsprojects.github.io/FSharp.Control.Reactive
Other
284 stars 58 forks source link

Order is not maintained in all cases of `Disposable.compose` #139

Closed mrakgr closed 4 years ago

mrakgr commented 4 years ago
/// Compose two disposables together so they are both disposed when disposed is called on the 'composite' disposable.
let compose (x : IDisposable) (d : IDisposable) =
    match d, x with
    | :? CompositeDisposable as d, x -> d.Add x; d :> IDisposable
    | d, (:? CompositeDisposable as x) -> x.Add d; x :> IDisposable
    | d, x -> let acc = new CompositeDisposable ()
                acc.Add d
                acc.Add x
                acc :> IDisposable

When freeing unmanaged resources sometimes the order matters. In every case but...

| d, (:? CompositeDisposable as x) -> x.Add d; x :> IDisposable

d is freed first and then x after that. Personally I'd flag this as a bug and just remove the line, but I am wondering what the others here think? Also, I'd add a comment to indicate which argument is disposed first and which last. I know that d has lower precedence than x in the alphabet which alludes to the order, but those inferences are a stretch.

deviousasti commented 4 years ago

There's another problem here. If either is a CompositeDisposable, it gets mutated.
I've used this pattern before -

let group1 = Disposable.compose dis1 dis2
...
let group2 = Disposable.compose group1 dis3

In this example, group1 would be mutated behind the scenes, and when it's disposed, dis3 would be called as well. I propose making compose pure, and add another ofSeq method to capture multiple disposables,

Factored this out to a separate issue. See #140

mrakgr commented 4 years ago

I agree that compose should be pure. Let me close this since implementing it in a pure fashion would resolve this. I missed that troublesome aspect of the way it is currently implemented. Well spotted.