Fix a leak in CurrentValueRelay.

[jasdeepsaini]

Issue 1 (commit)

Scenario 1

Depending on the order in which a CurrentValueRelay and its subscription are deallocated, the relay can leak its stored object.

This does not leak when the containing object is deallocated.

let relay = CurrentValueRelay(SomeObject())
let cancellables = Set<AnyCancellable>()

This leaks when the containing object is deallocated.

let cancellables = Set<AnyCancellable>()
let relay = CurrentValueRelay(SomeObject())

Other conditions can lead to the leak, but this is the easiest one to reproduce and test. Here's a project which produces the issue.

When I first found this issue, I thought it was a fluke, but the order in which objects are deallocated is deterministic but not guaranteed in Swift. Here's a swift issue which documents the behavior.

Here's what happens to cause the leak.

Original Declaration:

let cancellables = Set<AnyCancellable>()
let relay = CurrentValueRelay(SomeObject())

When the containing object is deallocated, cancellables is deallocated first. This causes cancel to be called on the subscription to the relay. Code

 func cancel() {
        sink = nil
}

Then the relay is deallocated. Code

deinit {
    // Send a finished event upon dealloation
    subscriptions.forEach { $0.forceFinish() }
}

This causes forceFinish to be called for the subscription, but sink is nil at this point, so nothing happens. Code

func forceFinish() {
       self.sink?.shouldForwardCompletion = true
       self.sink?.receive(completion: .finished)
}

At this point, the relay is deallocated, but the object it stored gets leaked.

This PR fixes the issue by calling forceFinish in the cancel method of the subscription. That forces the sink to clean up its memory.

The order of declarations determines how the stream terminates. The subscription will receive a finished event if the relay is deallocated first. If cancellable is deallocated first, the subscription will be canceled and will never receive a finished event.

This is the correct behavior. In the documentation for AnyCancellable, it says:

An AnyCancellable instance automatically calls cancel() when deinitialized.

Once a stream has been canceled, it won't accept any new events. So canceling the stream causes the finished event to be ignored.

Scenario 2 (Project has been updated to show this scenario)

If a withLatestFrom is added to the relay, two objects will be leaked if cancellables comes before the declaration of the initial relay.

This order of declarations causes a leak when the containing object is deallocated.

let cancellables = Set<AnyCancellable>()
let relay = CurrentValueRelay(SomeObject())
let withLatestFromRelay  = CurrentValueRelay(SomeObject())

relay
      .withLatestFrom(relay2)
      .store(in: &cancellables)

The flow is the same as Scenario 1, except setting the sink to nil causes the cancel method in withLatestFrom to be skipped. Code

func cancel() {
    sink?.cancelUpstream()
    sink = nil
    otherSubscription?.cancel()
}

Since the method is skipped, the value objects for relayandwithLatestFromRelay` leak.

Issue 2 (commit)

Fixing issue 1 led to a crash in the second scenario mentioned above. The original leak and crash caused by the fix were caught for the 'withLatestFrom' operator, but both could happen for any publisher that uses 'Sink.'

let cancellables = Set<AnyCancellable>()
let relay = CurrentValueRelay(SomeObject())
let withLatestFromRelay  = CurrentValueRelay(SomeObject())

relay
      .withLatestFrom(relay2)
      .store(in: &cancellables)

Now cancel() is called in the withLatestFrom operator which causes sink?.cancelUpstream() to be called. In this case, upstream is relay and has already been canceled. Code

func cancel() {
    sink?.cancelUpstream()
    sink = nil
    otherSubscription?.cancel()
}

The sink?.cancelUpstream call the cancelUpstream method in Sink. Code

func cancelUpstream() {
    upstreamSubscription.kill()
}

That leads to the kill method in DemandBuffer. Code

mutating func kill() {
    self?.cancel()
    self = nil
}

This method causes cancel to be called on relay, which was canceled to start this sequence of calls. Calling cancel twice on a Subscription causes a crash. The Apple Docs states, You can only cancel aSubscriptiononce.

[freak4pc]

Very nice catch, thanks!

[codecov[bot]]

Codecov Report

Merging #137 (c522cd1) into main (5818492) will decrease coverage by 0.03%. The diff coverage is 91.94%.

@@            Coverage Diff             @@
##             main     #137      +/-   ##
==========================================
- Coverage   95.53%   95.49%   -0.04%     
==========================================
  Files          68       70       +2     
  Lines        3833     4111     +278     
==========================================
+ Hits         3662     3926     +264     
- Misses        171      185      +14     

Impacted Files	Coverage Δ
Tests/CurrentValueRelayTests.swift	93.86% <91.17%> (-4.82%)	:arrow_down:
Sources/Common/Sink.swift	66.00% <100.00%> (+8.50%)	:arrow_up:
Sources/Relays/CurrentValueRelay.swift	94.28% <100.00%> (+0.16%)	:arrow_up:
Sources/Operators/PrefixWhileBehavior.swift	92.00% <0.00%> (ø)
Tests/PrefixWhileBehaviorTests.swift	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5818492...c522cd1. Read the comment docs.