diff --git a/Sources/Relays/PassthroughRelay.swift b/Sources/Relays/PassthroughRelay.swift index ba4260e..e0d5f1c 100644 --- a/Sources/Relays/PassthroughRelay.swift +++ b/Sources/Relays/PassthroughRelay.swift @@ -80,6 +80,7 @@ private extension PassthroughRelay { func forceFinish() { sink?.shouldForwardCompletion = true sink?.receive(completion: .finished) + sink = nil } func request(_ demand: Subscribers.Demand) { @@ -87,7 +88,7 @@ private extension PassthroughRelay { } func cancel() { - sink = nil + forceFinish() } } } diff --git a/Tests/PassthroughRelayTests.swift b/Tests/PassthroughRelayTests.swift index 6f946a4..c359059 100644 --- a/Tests/PassthroughRelayTests.swift +++ b/Tests/PassthroughRelayTests.swift @@ -143,5 +143,207 @@ class PassthroughRelayTests: XCTestCase { XCTAssertFalse(completed) XCTAssertEqual(values, ["initial", "1", "2", "3"]) } + + // MARK: - Memory Leak Tests (Issue #167, PR #168) + + // There was a race condition which caused subscriptions in PassthroughRelay + // to leak. Details of the race condition are in this PR: + // + // https://github.com/CombineCommunity/CombineExt/pull/168 + // + // The issue is similar to CurrentValueRelay (PR #137). The easiest way to + // reproduce the race condition is to initialize `cancellables` before `relay`. + // These tests confirm subscriptions are properly released regardless of + // initialization order. + + final class StoredObject { + nonisolated(unsafe) static var storedObjectReleased = false + + let value = 10 + + init() { + Self.storedObjectReleased = false + } + + deinit { + Self.storedObjectReleased = true + } + } + + final class StoredObject2 { + nonisolated(unsafe) static var storedObjectReleased = false + + let value = 20 + + init() { + Self.storedObjectReleased = false + } + + deinit { + Self.storedObjectReleased = true + } + } + + func testSubscriptionIsReleasedWhenRelayIsDeallocatedAndDeclaredAfterCancellables() { + final class ContainerClass { + nonisolated(unsafe) static var receivedCompletion = false + nonisolated(unsafe) static var receivedCancel = false + + // Cancellables comes before the relay + var cancellables = Set() + let relay = PassthroughRelay() + + init() { + relay + .handleEvents(receiveCancel: { + Self.receivedCancel = true + }) + .sink( + receiveCompletion: { _ in + Self.receivedCompletion = true + }, + receiveValue: { _ in } + ) + .store(in: &cancellables) + } + } + + var container: ContainerClass? = ContainerClass() + + XCTAssertFalse(ContainerClass.receivedCompletion) + XCTAssertFalse(ContainerClass.receivedCancel) + container = nil + XCTAssertNil(container) + + // Cancellables is deallocated before the relay, so cancel is called + XCTAssertFalse(ContainerClass.receivedCompletion) + XCTAssertTrue(ContainerClass.receivedCancel) + } + + func testSubscriptionIsReleasedWhenRelayIsDeallocatedAndDeclaredBeforeCancellables() { + final class ContainerClass { + nonisolated(unsafe) static var receivedCompletion = false + nonisolated(unsafe) static var receivedCancel = false + + // Relay comes before cancellables + let relay = PassthroughRelay() + var cancellables = Set() + + init() { + relay + .handleEvents(receiveCancel: { + Self.receivedCancel = true + }) + .sink( + receiveCompletion: { _ in + Self.receivedCompletion = true + }, + receiveValue: { _ in } + ) + .store(in: &cancellables) + } + } + + var container: ContainerClass? = ContainerClass() + + XCTAssertFalse(ContainerClass.receivedCompletion) + XCTAssertFalse(ContainerClass.receivedCancel) + container = nil + XCTAssertNil(container) + + // Relay is deallocated first, so completion is sent + XCTAssertTrue(ContainerClass.receivedCompletion) + XCTAssertFalse(ContainerClass.receivedCancel) + } + + func testStoredObjectsAreReleasedWithWithLatestFromAndDeclaredBeforeCancellables() { + final class ContainerClass { + nonisolated(unsafe) static var receivedCompletion = false + nonisolated(unsafe) static var receivedCancel = false + + // Relays come before cancellables + let relay = PassthroughRelay() + let relay2 = PassthroughRelay() + var cancellables: Set? = Set() + + init() { + relay + .withLatestFrom(relay2) + .handleEvents(receiveCancel: { + Self.receivedCancel = true + }) + .sink( + receiveCompletion: { _ in + Self.receivedCompletion = true + }, + receiveValue: { _ in } + ) + .store(in: &cancellables!) + + // Send initial values so withLatestFrom has something to work with. + relay2.accept(StoredObject2()) + } + } + + var container: ContainerClass? = ContainerClass() + + XCTAssertFalse(ContainerClass.receivedCompletion) + XCTAssertFalse(StoredObject.storedObjectReleased) + XCTAssertFalse(StoredObject2.storedObjectReleased) + + container = nil + XCTAssertTrue(StoredObject2.storedObjectReleased) + XCTAssertNil(container) + + // withLatestFrom keeps the relay subscription alive until cancellables are released. + XCTAssertFalse(ContainerClass.receivedCompletion) + XCTAssertTrue(ContainerClass.receivedCancel) + } + + func testStoredObjectsAreReleasedWithWithLatestFromAndDeclaredAfterCancellables() { + final class ContainerClass { + nonisolated(unsafe) static var receivedCompletion = false + nonisolated(unsafe) static var receivedCancel = false + + // Cancellables comes before the relays - this is the problematic case + var cancellables: Set? = Set() + let relay = PassthroughRelay() + let relay2 = PassthroughRelay() + + init() { + relay + .withLatestFrom(relay2) + .handleEvents(receiveCancel: { + Self.receivedCancel = true + }) + .sink( + receiveCompletion: { _ in + Self.receivedCompletion = true + }, + receiveValue: { _ in } + ) + .store(in: &cancellables!) + + // Send initial values so withLatestFrom has something to work with. + relay2.accept(StoredObject2()) + } + } + + var container: ContainerClass? = ContainerClass() + + XCTAssertFalse(ContainerClass.receivedCompletion) + XCTAssertFalse(StoredObject.storedObjectReleased) + XCTAssertFalse(StoredObject2.storedObjectReleased) + + // Setting container to nil deallocates cancellables first + // This should not crash and should properly release objects + container = nil + XCTAssertTrue(StoredObject2.storedObjectReleased) + XCTAssertNil(container) + + // Cancellables deallocated first, so cancel is called + XCTAssertFalse(ContainerClass.receivedCompletion) + XCTAssertTrue(ContainerClass.receivedCancel) + } } #endif