Fix: ensure connections are disposed for filter exception scenarios

[raccoonback]

Hello,
I am running a service using Spring Cloud Gateway based on reactor-netty, and recently, I became interested in the NettyWriteResponseFilter filter.

I think that the doOnError() method for connection disposal should be placed before the .then() call, not after.
This is because doOnError() operates downstream and does not have significant meaning when placed after .then().
Therefore, I propose moving the doOnError() configuration to a point before the .then() method.
Similarly, in the WebClientWriteResponseFilter filter, doOnError() is also set before .then() and handled in a comparable manner.
(As for doOnCancel(), since it operates upstream, it seems appropriate to set it after the .then() method.)

Thanks.

[spencergibb]

@chemicL can you comment on the semantics of this change?

[chemicL]

Hey,

I do not think this change is justified. Let's go step by step:

doOnError

Example

    chain.filter(exchange) // <A>
        .doOnError(throwable -> cleanup(exchange)) // <1>
        .then(Mono.defer(() -> ...) /* <B> */)
        .doOnError(throwable -> cleanup(exchange)) // <2>

Discussion

If <A> propagates an onError signal:

• Both <1> and <2> will observe it and take action
• <B> will not be subscribed to.

If <B> propagates an onError signal:

• <1> will not see it. <2> will.
• If <2> is not anywhere below in the chain, the error will not be acted upon.

I think the last point is the important bit and makes the proposal invalid regarding the doOnError move.

doOnCancel

In the proposal the position of this operator does not change. However it's worth considering what would happen if it was also moved.

Example

    chain.filter(exchange) // <A>
        .doOnCancel(() -> cleanup(exchange)) // <1>
        .then(Mono.defer(() -> ...) /* <B> */)
        .doOnCancel(() -> cleanup(exchange)) // <2>
        // <X>

If cancellation comes at level <X>:

• If <B> is being run, only <2> will trigger. That's because <A> is already in the terminated state.
• If <B> is not run yet, but <A> is still taking place, both <2> and <1> will get triggered.

If cancellation comes from executing <B>:

• <2> will not see it.
• <1> neither will since <A> chain and everything leading to then operator is in the terminated state.
• The only operators that will see cancellation are the currently running ones specified in ... upstream from the one triggering cancellation.

Therefore, it's worth analyzing where cancellation can happen and identify the point where the connection is in use. BTW I believe for this exact reason operators in the using and usingWhen families exist.

doFinally

For use cases where .doOnCancel and .doOnError are both used, consider replacing them with .doFinally:

    .doFinally(s -> {
        if (s == SignalType.CANCEL || s == SignalType.ON_ERROR) { ... }
    }

Here's how reactor-netty uses it. This should yield better performance.

Side comment

Having the above, that's still just theory. I believe any change should go with a set of tests that fail without the change being applied and pass once the change is applied. That's the only way to confirm the theory is sound, and reality proves it not always is :) So it's worth reporting issues to reactor-core in case some semantics are broken. Cheers!

[raccoonback]

@chemicL Thanks for your detailed explanation!
I was mistaken.
The existing doOnError(), doOnCancel() seems to work properly, and I think applying doFinally() is a good way.