Experiment for aborting callbacks

[gregw]

This is another experimental attempt to solve #11854 by adding abort semantic to all callbacks

[gregw]

The current status is that I have:

• added the API to Callback
• Done a basic implementation in Callback.Completing that substantially matches the API of IteratingCallback
• added a CallbackTest
• Started looking at IteratingCallback so that it fully supports the Callback.Completing contract, but with calls to onAbort and onCompleteFailure serialized

I think there is a bit of work to do, but once done in ICB, it should protect most of the rest of the codebase from these complexities.

[lorban]

@lorban wrote:

I have two concerns about this proposal:

• If the cancellable callback is wrapped by a legacy callback that does not override abort then this new aborting mechanism is bypassed and this fix is defeated; ContentSinkSubscriber, ContextResponse and probably more implementations that have this problem exist in our codebase. We may accept to live with this by fixing our broken callbacks, but I would at least review the known users of the Core API (Spring?) to fix their potential uses of "legacy wrapping callbacks". I've pushed a test to illustrate this.

If an abort(x) call is passed to a Callback wrapper that does not support it, then it is converted to an failed(x) call, so we ultimately are no worse than we are today. Luckily most of our callback composition is done with utility methods and base classes, so if we fix them, we should fix 95% of the use-cases... but I expect there might be a few callback implementations we need to track down and fix.

• Callback.Completing has 4 events: onAbort, onCompleteSuccess, onCompleteFailure and completed which I fail to understand. The standard pattern where the root problem has been found is that onCompleteSuccess and onCompleteFailure do some state transition logic, or delegate to another callback and release the buffer whose lifecycle is linked to the callback. The only thing that needs to change is to delay the release of the buffer when the callback is asynchronously aborted until succeeded or failed is called. So keeping the calls to onCompleteSuccess and onCompleteFailure untouched but adding an extra completed call that is delayed until the completion of the callback when the latter is aborted is what seems to me like the most natural fix. Here I fail to understand what onAbort() brings and when it's supposed to be called.

I've played with lots of different ways of breaking down the current single event into two events, including as you have suggested. I'm currently going with the current contract mostly based on naming. Because we already have a method called onCompleteFailure, then it makes sense to me that it is called when the callback is both failed and completed. We could of then had an onFailure(x) that was called on either an abort(x) or a failed(x), but that naming is a bit strange. Ultimately, once we have the split semantic working, I'm very happy to play around with exactly what methods exist and what names they have... I think we do have several options.... but coming up with naming that is consistent with what we currently have may influence us one way or the other. I.E. I'd like to stick with this contract for now, as t he naming makes the implementation clearer... then let's revisit once working

[gregw]

@lorban @sbordet I have reimplemented ICB now and also written some more extensive tests so that I call abort in every state for every action and result. Coverage is pretty good, but not perfect.

@lorban want to try to update HttpOutput to use this semantic again?

[gregw]

@lorban hmmm I've broken a few tests.... more than I would have expected..... Anyway, I'm tools down for the day. The branch is yours to play with... or to look at alternative APIs

[lorban]

@gregw I pushed the (minimal) necessary modifications needed to fix the IllegalArgumentException in SocketChannel.write() and I'm happy to report this branch now fixes the original problem.

As you can see, I've had to replace a new Callback() {...} anonymous class with a Callback.Nested and a couple of implements Callback with extends Callback.AbstractCallback, otherwise the "abort link" is severed, abort is transformed into fail and completed is called immediately after onCompleteFailure.

Conclusion: no one (not us, not our users) should ever do implements Callback nor new Callback() {...} ever again: we have to change all our implementations, look for 3rd party usages (Spring? others?), change our docs and clearly state in the Callback javadoc that directly implementing the interface is discouraged.

I'm even tempted to set the default abort() implementation in Callback to throw UnsupportedOperationException to avoid silently running into that problem in the future.

Oh, and I finally figured onAbort, onCompleteSuccess, onCompleteFailure and completed, and I like them. Now I just think completed should be renamed to onCompleted, and the javadocs should be reviewed as that was the cause of my confusion. No biggie.

[lorban]

I discussed the latest state with @sbordet and he raised two points:

• We need to add a serie of new Callback.from helpers that take 3 parameters, like: Callback.from(Runnable success, Consumer<Throwable> failure, Runnable complete) for our internal usage as well as for users of the Core API.
• Maybe we should leave Callback as it is and introduce a new Callback.Cancellable private subinterface instead? This way, we would need to add some instanceof checks to be able to abort, but we could detect when a cancellable callback is needed, and if not provided emit a warning linking to the issue/documentation explaining what needs to be changed.

[gregw]

I discussed the latest state with @sbordet and he raised two points:

• We need to add a serie of new Callback.from helpers that take 3 parameters, like: Callback.from(Runnable success, Consumer<Throwable> failure, Runnable complete) for our internal usage as well as for users of the Core API.

Sure - the more useful Helpers, then the less likely code is going to try to make their own.

• Maybe we should leave Callback as it is and introduce a new Callback.Cancellable private subinterface instead? This way, we would need to add some instanceof checks to be able to abort, but we could detect when a cancellable callback is needed, and if not provided emit a warning linking to the issue/documentation explaining what needs to be changed.

I don't see how that could work. We need to flow abort-then-complete semantics through all our callback chains. If Callback didn't have abort, then all those chains would be broken no matter what??? Essentially we have the implementation of abort only in Abstract, but the signature in Callback, so all implementations should be aware they need to do something.

[gregw]

Oh, and I finally figured onAbort, onCompleteSuccess, onCompleteFailure and completed, and I like them. Now I just think completed should be renamed to onCompleted, and the javadocs should be reviewed as that was the cause of my confusion. No biggie.

I'll change completed to onCompleted and just call the legacy completed in the default implementation.

For completeness, we could add a onAbortOrFailure that is called on either... but leaving out until clear that it is needed

[gregw]

Hmmm, I think I have found a use-case for having just a onFailure(Throwable) which is called immediately on abort or failure, in addition to onCompleteFailure.... I'm working on that now...

[lorban]

Hmmm, I think I have found a use-case for having just a onFailure(Throwable) which is called immediately on abort or failure, in addition to onCompleteFailure.... I'm working on that now...

What difference do you see between onFailure() and onCompleteFailure()? Isn't onCompleteFailure() already supposed to be called immediately on abort()?

[gregw]

Hmmm, I think I have found a use-case for having just a onFailure(Throwable) which is called immediately on abort or failure, in addition to onCompleteFailure.... I'm working on that now...

What difference do you see between onFailure() and onCompleteFailure()? Isn't onCompleteFailure() already supposed to be called immediately on abort()?

The onCompleteXxx methods are only called when the callback is complete, which may not need immediately on an abort

[lorban]

Friendly reminder that these changes may impact Spring's Jetty Core support.

[gregw]

Friendly reminder that these changes may impact Spring's Jetty Core support.

How can they affect Spring? At worst, abort(x) is not implemented in a callback, so turns into a failed(x) and will thus give exactly the same behaviour as now?

[lorban]

How can they affect Spring? At worst, abort(x) is not implemented in a callback, so turns into a failed(x) and will thus give exactly the same behaviour as now?

This is what I meant: we'll need to review the Spring PR to make sure this fix makes it into it too.

[gregw]

@sbordet @lorban I've updated for much of your feedback. Specifically simplifying the signature of Callback.Abstract and also fixing the onAbort race that it had. There are now numerous test failures, which I need to track down, but I'd appreciate your early review of this to make sure I'm heading in the right direction. @lachlan-roberts would be good if you looked at this as well, at least at the changes and tests for Callback.Abstract

[gregw]

@sbordet @lorban I think we have an issue with extensions of ICB that override succeeded(). Typically these overrides are releasing buffers and/or chunks, but now that we have a serialized onAbort(Throwable) method, they may be operating on the same fields in different threads with no memory barries. These implementations are typically in h2, h3 and quic. They all need to be rewritten to move all processing into one of the serialized methods (see #11932).

Also, almost all of our ICB usages need to be reviewed to make them correctly handle onAbort(Throwable). For now, I have made the default implementation of onAbort(Throwable) call onFailed(Throwable), so that we get the same fallback behaviour as Callback.

[gregw]

@sbordet I'm also now a bit cautious about the change you asked for, where it is onCompleted(Throwable) that calls onCompleteSuccess() or onCompleteFailure(Throwable). That means that overrides of onCompleted should call super. But now we have a fallback implementation in both Callback.abort(Throwable) and ICB.onAborted(Throwable) that means overrides MUST NOT call super. I'm inclined to revert to the code that never requires a call to super.

[gregw]

@lorban @sbordet I very much doubt this is going to be ready in time for 12.0.11. I think we should consider merging early in the 12.0.12 cycle or even only in 12.1.0

[gregw]

@lorban @sbordet remind me again why we can't resolve this issue by removing buffers from a pool?

Currently we have to revisit every callback that does a release and make sure it separates out abort from failed behaviour.... Why not instead, revisit every callback that does a release and make sure that it does a remove of the buffer from the pool in any failed write?

I think that is going to be less invasive and can be done simply in 12.0.x without revolution.

Note that I think there are some good generalizations of abort in this PR, as well as some good fixes to ICB, but I think it would be a lot simpler if it didn't delay complete failure waiting for a succeeded/failed callback. We can do these in 12.1.x

[gregw]

@lorban see #11951 as an alternate solution

[lorban]

@gregw we considered removing the buffer from the pool, but the solution was dismissed because it looked as complex as introducing Callback.abort(): it either requires a new RBB.remove() call or to make sure that every place that figures removal must occur has to have a ref to the buffer pool.

Looking back, it seems we underestimated the complexity of Callback.abort() so removing the buffer from the pool now looks like a better solution for 12.0.x.