qianye1001 commented on issue #10745: URL: https://github.com/apache/rocketmq/issues/10745#issuecomment-5564686716
Before treating this as a confirmed resource leak, could you please provide a minimal reproduction showing that one side remains active after the relevant close futures and event-loop tasks have completed? The current code appears to implement paired cleanup through a `channelInactive` cascade: - In `Http2ProxyFrontendHandler.channelRead`, the write is performed on `outboundChannel`. Therefore `future.channel()` is the outbound/backend channel. If that write fails, closing it should invoke `Http2ProxyBackendHandler.channelInactive`, which closes `inboundChannel`. - In `Http2ProxyBackendHandler.channelRead`, the write is performed on `inboundChannel`. Therefore `future.channel()` is the inbound/frontend channel, not the backend channel as stated in the automated issue evaluation. Closing it should invoke `Http2ProxyFrontendHandler.channelInactive`, which closes `outboundChannel`. - The two `exceptionCaught` paths close their current `ctx.channel()`; the subsequent `channelInactive` callback closes the peer. The linked Stack Overflow answer is about duplicating traffic from one inbound connection to two outbound servers. Its sample still closes only `future.channel()` on a write failure and only `ctx.channel()` in `exceptionCaught`; associated outbound channels are closed from `channelInactive`. It therefore does not appear to recommend explicitly closing both sides in each failure callback: https://stackoverflow.com/a/42681543 The tests added in #11052 use mocked channels and verify that two explicit close calls were made. They do not connect the frontend and backend handlers into paired pipelines or execute the real `channelInactive` lifecycle. Consequently, they fail on the old implementation because they encode the proposed implementation detail, not because they demonstrate that a channel remains active or that a resource is retained. A useful regression test would run against the unmodified `develop` implementation with paired `EmbeddedChannel` instances or real local socket channels, wait for or drain the close futures and pending event-loop tasks, and then demonstrate that exactly one side is still active. It would also help to identify the concrete condition that prevents `channelInactive` from reaching the peer-closing handler. If such a reproduction exists, the explicit cleanup may be justified. Without it, this looks more like defensive or redundant hardening than a demonstrated connection-leak bug. The direct `close()` additions on write failure also change the existing `closeOnFlush()` shutdown behavior for the peer, so that semantic change should be justified by the reproduction. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
