Suggests to dispose connection when cancelled In NettyWriteResponseFilter

[raccoonback]

Hello.
I use spring-gateway, reactor-netty to operate the service.
While investing "LEAK: ByteBuf.release() was not called before it's garbage-collected", I would like to ask for suggestions on fixing NettyWriteResponseFilter.

NettyWriteResponseFilter disposes only when the connection channel is active.

However, it seems that FluxReceive may result in unreleased memory references. (ChannelOperations)
Therefore, I suggest modifying the configuration to always dispose of the connection regardless of the channel status.
(I understand that there is no redundant processing for dispose already inside the connection.)

For example, suppose that an incoming HTTP request was canceled while routing and responding.
Even though the NioSocketChannel channel is not active, a memory reference may remain in receiverQueue maintained in FluxReceive.

Actually, if delay 5 seconds with a custom GlobalFilter and generate a large number of HTTP cancels after 2 seconds of the total request time, "LEAK: ByteBuf.release() was not called before it's garbage-collected" occurred.

@violetagg
hello.
What do you think about this?

[raccoonback]

Additionally, I have also reflect the feedback from this review.

[violetagg]

@raccoonback

However, it seems that FluxReceive may result in unreleased memory references.

I would say that if this is the case we need to fix this in Reactor Netty. Please provide a reproducible scenario!
In Reactor Netty we detect when the connection is closed or the request/response is canceled and we do make a cleanup.

Here this call back was added IIRC in order to handle the scenario when the connection 1 is closed so that we close connection 2

client -> Spring Cloud Gateway -> target server
       ^                        ^
connection 1                connection 2

[raccoonback]

@violetagg
Thanks good comment!
As your comment, if connection 1 is canceled, ChannelOperations.dispose() provided by Reactor Netty will be called, disposing of both the FluxReceiver and the PooledConnection, thereby releasing them.

Upon closer inspection, the disposal process follows the flow outlined below when connection 1 is canceled.

connection 1 cancel → NettyWriteResponseFilter.cleanup() → ChannelOperations.dispose() → FluxReceiver.dispose() and PooledConnection.dispose()

|              spring gateway area                       |                       reactor netty area                                         |

However, in NettyWriteResponseFilter, when connection 1 is canceled, it only attempts to dispose of the FluxReceiver and PooledConnection if the channel(NioSocketChannel) of connection 2 is active (connection.channel().isActive()).

connection 1 cancel → NettyWriteResponseFilter.cleanup() -/-> ChannelOperations.dispose()  

So.. "FluxReceiver.dispose() is not called, so the memory is not released." 

Therefore, if connection 2 is not in an active state and there are unreleased memory references in the FluxReceiver,
the dispose request might not be propagated to the FluxReceiver even when connection 1 is canceled.
(because NettyWriteResponseFilter.cleanup() propagates dispose only when the channel in connection 2 is active.)

As a result, it seems that NettyWriteResponseFilter should ensure that the dispose request is propagated to the FluxReceiver and PooledConnection regardless of the active state of connection 2.

Do you agree that NettyWriteResponseFilter should propagate the dispose request to FluxReceiver and PooledConnection regardless of the channel's active state?

[violetagg]

Therefore, if connection 2 is not in an active state and there are unreleased memory references in the FluxReceiver

We invoke dispose when the connection is closed (the channelInactive event) -> this means that if the connection is not active then the dispose has been already invoked.

[violetagg]

@raccoonback Actually ignore my previous comment! You are right, for the client we will keep the buffered data as there are scenarios when data consumption might be delayed!

[raccoonback]

@violetagg Thanks for your review! 😃

[raccoonback]

@spencergibb
Hello!
Please give a review based on the above information~

[raccoonback]

@spencergibb
Hello!
Could you please take a look at this PR?
The review seems to be delayed, so I would greatly appreciate your confirmation.
Thank you!