Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix async callback failure leading to IllegalArgumentException in SocketChannel.write() #11860

Draft
wants to merge 4 commits into
base: jetty-12.0.x
Choose a base branch
from

Conversation

lorban
Copy link
Contributor

@lorban lorban commented May 31, 2024

Prototype fix for #11854

This is a slightly modified version of the original proposal that does not require any modification to the Callback interface: introduce a new CancelableCallback class with a static cancel(Callback, Throwable) helper that actually calls failed() on the callback but not before wrapping the given throwable with a private, maker one that is then detected by the CancelableCallback.failed() implementation to discriminate between canceling and failing logic.

…menting async cancelable callbacks

Signed-off-by: Ludovic Orban <[email protected]>
@lorban
Copy link
Contributor Author

lorban commented May 31, 2024

See #11857 for an alternative which adds a cancel() method to Callback.

…menting async cancelable callbacks

Signed-off-by: Ludovic Orban <[email protected]>
…menting async cancelable callbacks

Signed-off-by: Ludovic Orban <[email protected]>
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lorban, whilst I agree the other approach (modifying Callback) is very invasive (and has yet been worked all the way through), I'm not sure this approach is feasible.

The issue is that we frequently wrap Callbacks and such a wrapper between the callback being cancelled and the one being cancelled may not know about cancellation. It would thus see an initial call to fail(Throwable) and would not notice that the exception is specially wrapped. It should pass that on to its nested callback OK, but then it would be within the contract of Callback to become inactive and ignore all subsequent calls to succeeded or failed. Thus there is no guarantee that the ultimate completion signal would be passed on.

I can't see how we can avoid modifying Callback to give wrappers the obligation to pass on both the cancelled event and the ultimate completion (succeeded or failed) event.

Since many of our Callback wrappers are generated by utility methods/classes, this should limit the impact of this. Note that IteratingNestedCallback actually already has abort(Throwable), which a step in this direction.

At the very least, this approach would need a change in the Callback contract to say that multiple failed(Throwable) calls should always be passed on to any nested callback. Once we say that, is it really any different to adding a new cancel method that must be passed on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants