From ba0b75f5a0a9f828480ebb7bead83b57fc70d903 Mon Sep 17 00:00:00 2001 From: just1103 Date: Sun, 11 Feb 2024 00:21:41 +0900 Subject: [PATCH 1/2] fixed memory leak with PassthroughRelay --- Sources/Relays/PassthroughRelay.swift | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Sources/Relays/PassthroughRelay.swift b/Sources/Relays/PassthroughRelay.swift index ba4260e..c683420 100644 --- a/Sources/Relays/PassthroughRelay.swift +++ b/Sources/Relays/PassthroughRelay.swift @@ -78,8 +78,9 @@ private extension PassthroughRelay { } func forceFinish() { - sink?.shouldForwardCompletion = true - sink?.receive(completion: .finished) + self.sink?.shouldForwardCompletion = true + self.sink?.receive(completion: .finished) + self.sink = nil } func request(_ demand: Subscribers.Demand) { @@ -87,7 +88,7 @@ private extension PassthroughRelay { } func cancel() { - sink = nil + forceFinish() } } } From 7dbb1664fc5ff24db77e769ab62bd7004e244ba4 Mon Sep 17 00:00:00 2001 From: Shai Mishali Date: Tue, 20 Jan 2026 20:55:04 +0200 Subject: [PATCH 2/2] Add comprehensive memory leak tests for PassthroughRelay - Test subscription release with different initialization orders - Test withLatestFrom operator scenarios - Mirrors test coverage from CurrentValueRelay (PR #137) - Verifies fix for issue #167 --- Sources/Relays/PassthroughRelay.swift | 6 +- Tests/PassthroughRelayTests.swift | 202 ++++++++++++++++++++++++++ 2 files changed, 205 insertions(+), 3 deletions(-) diff --git a/Sources/Relays/PassthroughRelay.swift b/Sources/Relays/PassthroughRelay.swift index c683420..e0d5f1c 100644 --- a/Sources/Relays/PassthroughRelay.swift +++ b/Sources/Relays/PassthroughRelay.swift @@ -78,9 +78,9 @@ private extension PassthroughRelay { } func forceFinish() { - self.sink?.shouldForwardCompletion = true - self.sink?.receive(completion: .finished) - self.sink = nil + sink?.shouldForwardCompletion = true + sink?.receive(completion: .finished) + sink = nil } func request(_ demand: Subscribers.Demand) { 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