Issues with RetryWhen

[Kn1kt]

Now RetryWhen subscribes to upstream twice. This is because the Sink itself subscribes to the upstream in .init.
Test testSuccessfulRetry() checks the number of subscriptions, but not correctly. It should check that the error publisher produces at least one item.

The correct test should look like this:

func testSuccessfulRetry() {
        var times = 0
        var retriesCount = 0
        
        var expectedOutput: Int?
        
        var completion: Subscribers.Completion<RetryWhenTests.MyError>?

        subscription = Deferred(createPublisher: { () -> AnyPublisher<Int, MyError> in
            defer { times += 1 }
            if times == 0 {
                return Fail<Int, MyError>(error: MyError.someError).eraseToAnyPublisher()
            }
            else {
                return Just(5).setFailureType(to: MyError.self).eraseToAnyPublisher()
            }
        })
        .retryWhen { error in
            error
                .handleEvents(receiveOutput: { _ in retriesCount += 1})
                .map { _ in }
        }
        .sink(
            receiveCompletion: { completion = $0 },
            receiveValue: { expectedOutput = $0 }
        )

        XCTAssertEqual(
            expectedOutput,
            5
        )
        XCTAssertEqual(completion, .finished)
        XCTAssertEqual(times, 2)
        XCTAssertEqual(retriesCount, 1)
    }

A possible solution is to remove upstream subscription from Sink.init and setting upstreamIsCancelled cancelUpstream function.

private let lock = NSRecursiveLock()


/// ...

func cancelUpstream() {
    lock.lock()
    guard !upstreamIsCancelled else {
        lock.unlock()
        return
    }
    upstreamIsCancelled = true
    lock.unlock()
    
    upstreamSubscription.kill()
}

[freak4pc]

Mind taking a look? @danielt1263

[danielt1263]

I'll have a look this weekend.

[danielt1263]

Good catch @Kn1kt. I have updated the test based on your suggestion and updated the code to make the test pass while still passing the other tests. See PR #150

[Kn1kt]

Good job. I also made a couple of fixes that can be merged after your PR. #151

[BerkeleyTrue]

Just had this bite me in the rear with API requests. @danielt1263 's PR fixed my issue.

[emixb]

I also spent some time debugging this issue, can we merge @danielt1263's PR? Thanks!

[doxuto]

@freak4pc Pls check @danielt1263 's PR. Thanks

[Kn1kt]

Looks like this PR stuck. I have improved this operator, you can check out it here: CombineKit. Hope this helps