On Wed, 22 Oct 2025 12:20:41 GMT, Daniel Fuchs <[email protected]> wrote:
> Could we remove the `preTerminationCallback` parameter, and instead have the > various subclass of `HttpBodySubscriberWrapper` supply > `this.multi::cancelTimer()` to their super class? > > If I'm not mistaken all concrete subclasses of `HttpBodySubscriberWrapper` > have access to the multi exchange. I had considered this approach, but had dropped it mainly because 1. `MultiExchange::cancelTimer` is a private method, and it would ideally remain so. The timer lifecycle, including cancellation, should be governed by `MultiExchange` itself, not by its subclasses. 2. The current flow makes it easy to trace where cancellation occurs by following method calls: `MultiExchange` →`Exchange::readBodyAsync` → `createResponseSubscriber`, etc. If we invert the logic, identifying where `cancelTimer` is invoked would become less transparent and would rely on IDE assistance. > That should reduce the amount of changes significantly. I can relate to that motivation. However, when adapting the code to let `HBSWrapper` subclasses call `cancelTimer`, the only files spared from modification would be `Exchange` and `ExchangeImpl`. So the overall reduction in changed lines would be minor. > Another possibility is to introduce a noarg protected abstract method in > `HttpBodySubscriberWrapper` ... That approach would face the same downsides as letting subclasses directly call `MultiExchange::cancelTimer`. > It might need to be called on cancel as well? If not a comment explaining why > might be good to have. If it's not called by cancel then maybe the method > suggested above could be changed to `onCompletionNotified`. That was indeed an oversight – fixed in 5621d911944. @dfuch, how would you prefer I proceed? Should I make `cancelTimer` public and pass it from `HBSWrapper` subclasses, or keep the current design? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27469#discussion_r2461364377
