CombineLatest sometimes completes early when an input observable is empty

[dominicfreeston]

Short description of the issue:

As discussed in a previous issue, the CombineLatest operator should only complete when all its input observables have completed, even if some of the observables are empty.

NB: This can cause situations where a value is never emitted and the observable never completes, but this was considered the correct behaviour as it's still possible for the observable to error out. (Even if the alternative is desired, i.e. that we complete as soon as an empty input observable completes, then we have a slightly different problem).

Slack thread where the issue was discussed: https://rxswift.slack.com/archives/C051G5Y6T/p1620141871228600

Expected outcome:

CombineLatest should not complete until all its input observable have completed (or error out when one of its observable does).

What actually happens:

CombineLatest completes after a value is received if all other observables have completed and at least one of them didn't emit a value.

Self contained code example that reproduces the issue:

In this example, both observables should error out. As seen, one of them errors out and the other one completes. Thanks to @danielt1263 for helping me narrow down the issue and create this example:

class rx_sandboxTests: XCTestCase {
    func test() throws {
		let scheduler = TestScheduler(initialClock: 0)
		let a = scheduler.createHotObservable([Recorded<Event<Int>>.error(230, NSError(domain: "", code: -1, userInfo: nil))])
		let b = scheduler.createHotObservable([Recorded<Event<String>>.completed(220)])
		let result = scheduler.start {
			Observable.combineLatest(a, b) { Result(a: $0, b: $1) }
		}
		XCTAssertEqual(result.events, [.error(230, NSError(domain: "", code: -1, userInfo: nil))])
	}
	func test2() throws {
		let scheduler = TestScheduler(initialClock: 0)
		let a = scheduler.createHotObservable([.next(220, 5), .error(230, NSError(domain: "", code: -1, userInfo: nil))])
		let b = scheduler.createHotObservable([Recorded<Event<String>>.completed(210)])
		let result = scheduler.start {
			Observable.combineLatest(a, b) { Result(a: $0, b: $1) }
		}
		XCTAssertEqual(result.events, [.completed(220)])
	}
}
struct Result: Equatable {
	let a: Int
	let b: String
}

RxSwift/RxCocoa/RxBlocking/RxTest version/commit

RxSwift 6.1.0

Platform/Environment

• [x] iOS
• [ ] macOS
• [ ] tvOS
• [ ] watchOS
• [ ] playgrounds

How easy is to reproduce? (chances of successful reproduce after running the self contained code)

• [x] easy, 100% repro
• [ ] sometimes, 10%-100%
• [ ] hard, 2% - 10%
• [ ] extremely hard, %0 - 2%

Xcode version:

12.4

Installation method:

• [x] CocoaPods
• [ ] Carthage
• [ ] Git submodules

I have multiple versions of Xcode installed: (so we can know if this is a potential cause of your issue)

• [ ] yes (which ones)
• [x] no

Level of RxSwift knowledge: (this is so we can understand your level of knowledge and formulate the response in an appropriate manner)

• [ ] just starting
• [ ] I have a small code base
• [x] I have a significant code base

[dominicfreeston]

FWIW if the basic premise is right (i.e. it should only complete when all inputs have completed) then I believe it's just a question of deleting the else branch in next.

[freak4pc]

Wow, I'm not sure how I missed your issue. Is this still relevant for you in any way?

I will say that - regardless of this being a bug or not (worth checking), it is not likely we could change the existing behavior since there are too many existing code bases probably counting on it.

[danielt1263]

Would it though? The basic premise is something that I think is generally accepted (that the operator doesn't complete until all inputs have completed) so I doubt that there is actually any code that relies on that not being the case. Also, the current behavior flatly contradicts the documented behavior of the operator. The combineLatest operator is supposed to emit the latest next value from every component when any of them emit a next event. Having a special case that forces it to short circuit that behavior is so obviously wrong...

[freak4pc]

Would it though? The basic premise is something that I think is generally accepted (that the operator doesn't complete until all inputs have completed) so I doubt that there is actually any code that relies on that not being the case. Also, the current behavior flatly contradicts the documented behavior of the operator. The combineLatest operator is supposed to emit the latest next value from every component when any of them emit a next event. Having a special case that forces it to short circuit that behavior is so obviously wrong...

I don't know much, but from years of API design and OSS work I do know that if an edge case exists, users will find it :)

Given this unexpected behavior is there for around 4 major releases, I'd assume there quite a few developers that might be accidentally counting on it or using it.

Changing this behavior will have to be a 7.0 kind of thing, unfortunately.

[danielt1263]

I think it's fine to do it for 7.0...

[dominicfreeston]

@freak4pc No problem, and thanks for getting back to me once you came across the issue. I no longer work on the codebase where I came across this issue and I was able to work around it at the time anyway. And I totally understand wanting to wait for a major version to change behaviour.

However, one thing I'd like to highlight from my original post in Slack:

Say I have a CombineLatest of two observables, if one completes without a value then a second sends a value, it will actually complete immediately when that value is received, even though the second observable has not completed! However, if one sends an event but does not complete then another one completes without a value, it will not complete, at least not until the first sends another value or completes.

Basically, I think either combineLatest should complete as soon as one of its inputs completes without a value (the behaviour I initially expected) or it should only complete when all inputs have completed (the behaviour you settled on in issue #2211).

At the moment we have a strange in-between where the same two events (complete in observable A and a next value in observable B) will cause the combineLatest to either hang forever or complete immediately depending on the timing of those two events. So even if people are currently relying on it completing early, they probably can't do so reliably.

Hope this helps and thanks for all your work!