Reactor-core: FluxSink's confusing onDispose and onCancel callbacks.

Created on 14 Jun 2018  路  4Comments  路  Source: reactor/reactor-core

For a regular Flux, when Disposable.dispose is called, it invokes the doOnCancel operator;
so there's a relationship between dispose and cancel. FluxSink however, provides two callback APIs onDispose and onCancel and adds confusion on their usage.

typdocumentation typenhancement

Most helpful comment

Here is what I had in mind:

  • keep doOnCancel as is (that's the most specific handler)
  • deprecate doOnDispose
  • add doOnTerminate
  • doOnTerminate has the implementation of current's doOnDispose, doOnDispose calls doOnTerminate. Replace usages of doOnDipose() with doOnTerminate()
  • only keep a single default void doOnDispose in the sink interfaces that delegates to doOnTerminate
  • deprecate isCancelled
  • add isTerminated, make sure the javadoc mentions all terminations, not only cancellation
  • replace all usages and implementations of isCancelled with isTerminated
  • only keep a default boolean isCancelled() in the sink interfaces that delegates to isTerminated

All 4 comments

I would add here that this issue more related to the lack of the documentation rather than to implementation, so it would be nice if the result of that discussion will be some agreement on the additional section for behavior on disposing of the Flux. Currently, there is a small section which describes the difference between FluxSink#onCancel FluxSink#onDispose.

I had a discussion with Simon Basle on this and he feels onDispose should rather be called onTerminate to make better sense.

Here is what I had in mind:

  • keep doOnCancel as is (that's the most specific handler)
  • deprecate doOnDispose
  • add doOnTerminate
  • doOnTerminate has the implementation of current's doOnDispose, doOnDispose calls doOnTerminate. Replace usages of doOnDipose() with doOnTerminate()
  • only keep a single default void doOnDispose in the sink interfaces that delegates to doOnTerminate
  • deprecate isCancelled
  • add isTerminated, make sure the javadoc mentions all terminations, not only cancellation
  • replace all usages and implementations of isCancelled with isTerminated
  • only keep a default boolean isCancelled() in the sink interfaces that delegates to isTerminated

Following discussion with @smaldini in @OlegDokuka's PR, this is maybe not the best course of action. The terminate(d) term is used throughout the codebase to mean onError or onComplete, and doesn't include cancel. disposed covers all three, hence the term.

The original confusion stems from confusing the Sink and the Subscription:

  • someFlux.subscribe().dispose() is actually disposing the Subscription, which is the same as cancelling it
  • for a Sink, disposed means no more data can be pushed to it. And a Sink is unusable as soon as it is completed (onComplete), failed (onError) or cancelled (cancel), hence that handler covers all three cases.

~I will propose a PR to improve the documentation~ See PR: #1249

Was this page helpful?
0 / 5 - 0 ratings