Single onCancel and OnComplete fixes

[danielkec]

Thx @olotenko for finding the issue
Signed-off-by: Daniel Kec daniel.kec@oracle.com

[olotenko]

This is an unsatisfactory change.

If a 5-minute drive-by review finds 3 issues, what else is lurking?

What makes the rewrite of the entire CompletableFuture necessary in the first place? Its operation and contract is subtle.

Type-wise problem:

Default Single.onCancel creates a new Single and was probably designed for builder-like flowing style. A subtype of Single now mutates this. Case in point: the very invocation of single.onCancel from inside onSubscribe is invalid; it only works as long as Single.from returns a specific subtype of Single. When that changes, this implementation will be broken and you will never find out why.

Atomicity:

On at least one path multiple things are modified, but not atomically. This is going to create problems in valid use cases. Case in point: multiple Subscriber will be created and subscribed, and will not make any progress, thus breaking the reactive spec that the Subscriber s eventually either cancel or request.

Deferred action:

Cancellation is deferred until onCancel is registered. (Ok, this one is addressed)

In order to justify a deep analysis of the change (#1664), I would like to see a justification for a rewrite in that pull request.

[tomas-langer]

Why did we rewrite Single as an implementation of CompletionStage:

1. Single is a promise of a single value, most of reactive implementations treat this as a specific Flow.Publisher case, yet it is a CompletionStage that can act as a publisher as well
2. Lot of implementation do not handle Flow (or it is too complicated to handle subscribers) for Single cases - a lot of times the API of CompletionStage is more suitable to implement desired functionality
3. Helidon APIs now have a single method, and it is a choice of user whether to use Single, Flow.Publisher or CompletionStage for a promise of a single unit. This makes the design and used of the API much easier, as before we needed to call additional methods

[olotenko]

How could it have "missed" onComplete? It cannot transition to any new state after super.complete, even if upstream decides to onError. Besides, the atomic getAndSet will transition to yet another state where the onComplete is futile, even if observed.

[danielkec]

@olotenko There is no onComplete signal coming from publisher if you send cancel from onNext

[olotenko]

So?

[danielkec]

Single.just("a").onComplete(()-> this is missed).await() commit message mentions operator onComplete not the subscriber's method

[olotenko]

You've just shown yet another problem with this design where both CompletionStage and Subscriber contracts overlap. Exactly what I told you offline.

Requesting cancellation is legitimate. Anyone who has access to Single or any other part of the pipeline can do that. If this breaks await, the design is wrong.

The problem with me enumerating what can go wrong is that it seems the specific symptoms get addressed, not the underlying design problem.