Discard vs. Drop

[OlegDokuka]

problem

It seems that similar issues were raised in the past, but I feel I'm not convinced enough to agree.

1. It seems (from @smaldini words from Better names for onNextDropped vs doOnDiscard hooks, compose/transform, doOnEach #1381) that onNextDropped is a spec violation which is incorrect.

   1. Let's consider FluxTake. It does take N elements, cancel subscription upon receiving the expected number of elements to take, and set the state to done. It means that in the case of racing between CANCEL and ON_NEXT more signals can appear (imaging .cancelOn up the stream which delays a little bit cancellation) which leads that few elements can be dropped easily (which is absolutely valid case due to 2.8).
   2. In the reactor, we are over protective so all the way we double-check different flags (either cancel or done) to ensure we are not sending something extra and dropping redundant.
   3. Taking into account everything said above, onNextDropped IS DESIGNED TO HANDLE unexpected along with racing between cancellation and onNext signals.

2. Now we have another operator to handle filtered (discarded) elements on the level of the local stream, BUT it has significant design FLAW:

   1. Hooks are localized, so the discarded element is consumed only if the hook is present.
   2. It is SUPER expensive in terms of performance when it comes to Mono(or even from flux perspective) where we constantly have 3-4 objects overhead (FluxContextStart + ContextStartSubscriber + Consumer<? super R> discardHook + created Context) is WAY expensive (💔)

proposal

The only way to make it consistent is to merge both drop and discard APIs and make them interconnected, e.g if no local hooks discard hooks installed, we are free to lookup for global hooks. Since Operators.onNextDrop is, let's call it, internal API, I don't see any problem to UNIFY that in 3.4

[simonbasle]

Unfortunately, it seems too late to address that one in time for 3.4, considering how late we already are in the pre-release cycle. Thus I'm moving this to 3.5 planning.

[cavallium]

Any update?

[OlegDokuka]

@simonbasle @chemicL I think it is a good oportunity to deprecate Operators.onNextDropped for 3.5.0 while making it private in 3.6

[simonbasle]

a bit of context on #3229 being closed (as discussed off-band): this wasn't likely addressing the right issue or using the right angle.
the onNextDropped hook should be single-responsability: a way to track malformed onNext signals. It shouldn't double as a way to discard value, which is a responsability of onDiscard. but one issue is that there is no global hook for onDiscard.

so a preferable solution would be to introduce a dedicated global hook in Hooks that Operators.onDiscard can fallback on when no hook is defined locally in the Context. then, make sure that wherever onNextDropped is called, Operators.onDiscard is also called. => #3240

finally, convey the message that global Hooks.onNextDropped shouldn't be used for discarding, and to prefer Hooks.onDiscard. if onNextDropped continues to be used for discard AND onDiscard hook is set up, the double discarding shouldn't be an issue as such hooks are required to be idempotent already.

[chemicL]

It's been a while and me and @violetagg are not convinced by this proposal. The assumption of discarding/dropping being idempotent would most probably not hold in reference counted objects as there is no checking of who and why is doing the decrement. That could invalidate further processing. Therefore, it feels this idea seems dangerous. The API and the behaviour have been considered stable for quite some time and it is a very risky area to touch. If new requirements arise we can come back to this idea.