HTTP2 async failure on a stream can lead to the connection being aborted #11854
Comments
lorban
added
Bug
|
@sbordet @lorban I think we could use the callback below. The /**
* A Callback that can be cancelled.
* The callback is cancelled when it is known that it cannot be completed successfully, but a scheduled callback to
* {@link #succeeded()} or {@link #failed(Throwable)} cannot be aborted.
* The following methods can be extended:
* <ul>
* <li>{@link #onSuccess()} will be called by a call to {@link #succeeded()} if the Callback has not already been
* {@link #cancel(Throwable) cancelled}.</li>
* <li>{@link #onFailure(Throwable)} will be called by a call to {@link #failed(Throwable)} if the Callback has not
* already been {@link #cancel(Throwable) cancelled}.</li>
* <li>{@link #onComplete(Throwable)} will always be called once either {@link #succeeded()} or
* {@link #failed(Throwable)} has been called. If passed {@link Throwable} will be null only if the callback has
* neither been {@link #cancel(Throwable) cancelled} nor {@link #failed(Throwable) failed}.</li>
* </ul>
*/
class CancelableCallback implements Callback
{
private static final Throwable SUCCEEDED = new StaticException("Succeeded");
AtomicMarkableReference<Throwable> _completion = new AtomicMarkableReference<>(null, false);
/**
* Cancel or fail a callback.
* @param callback The {@link Callback} to {@link #cancel(Throwable)} if it is an instance of {@code CancelableCallback},
* else it is {@link #failed(Throwable) failed}.
* @param cause The cause of the cancellation
*/
static void cancel(Callback callback, Throwable cause)
{
if (callback instanceof CancelableCallback cancelableCallback)
cancelableCallback.cancel(cause);
else
callback.failed(cause);
}
/**
* Cancel the callback if it has not already been completed.
* The {@link #onFailure(Throwable)} method will be called directly by this call, then
* the {@link #onComplete(Throwable)} method will be called only once the {@link #succeeded()} or
* {@link #failed(Throwable)} methods are called.
* @param cause The cause of the cancellation
*/
public void cancel(Throwable cause)
{
if (cause == null)
cause = new CancellationException();
if (_completion.compareAndSet(null, cause, false, false))
onFailure(cause);
}
@Override
public final void failed(Throwable failure)
{
if (failure == null)
failure = new Exception();
while (true)
{
if (_completion.compareAndSet(null, failure, false, true))
{
try
{
onFailure(failure);
}
finally
{
onComplete(null);
}
return;
}
if (_completion.isMarked())
return;
Throwable cause = _completion.getReference();
ExceptionUtil.addSuppressedIfNotAssociated(cause, failure);
if (_completion.compareAndSet(cause, cause, false, true))
{
onComplete(cause);
return;
}
}
}
@Override
public final void succeeded()
{
while (true)
{
if (_completion.compareAndSet(null, SUCCEEDED, false, true))
{
try
{
onSuccess();
}
finally
{
onComplete(null);
}
return;
}
if (_completion.isMarked())
return;
Throwable cause = _completion.getReference();
if (_completion.compareAndSet(cause, cause, false, true))
{
onComplete(cause);
return;
}
}
}
/**
* Called when the callback has been {@link #succeeded() succeeded} but not {@link #cancel(Throwable) cancelled}.
* The {@link #onComplete(Throwable)} method will be called with a {@code null} argument after this call.
* Typically, this method is implement to act on the success. It can release or reuse any resources that may have
* been in use by the scheduled operation, but it should defer that release or reuse to the subsequent call to
* {@link #onComplete(Throwable)} to avoid double releasing.
*/
protected void onSuccess()
{
}
/**
* Called when the callback has either been {@link #failed(Throwable) failed} or {@link #cancel(Throwable) cancelled}.
* The {@link #onComplete(Throwable)} method will ultimately be called, but only once the callback has been
* {@link #succeeded() succeeded} or {@link #failed(Throwable)}.
* Typically, this method is implemented to act on the failure, but it cannot release or reuse any resources that may
* be in use by the schedule operation.
* @param cause The cause of the failure or cancellation
*/
protected void onFailure(Throwable cause)
{
}
/**
* Called once the callback has been either {@link #succeeded() succeeded} or {@link #failed(Throwable)}.
* Typically, this method is implemented to release resources that may be used by the scheduled operation.
* @param cause The cause of the failure or cancellation
*/
protected void onComplete(Throwable cause)
{
}
} |
|
thought bubble for this is in #11857. A fair bit more work is needed... and I'm not entirely convinced it is doable yet. |
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
…menting async cancelable callbacks Signed-off-by: Ludovic Orban <lorban@bitronix.be>
…menting async cancelable callbacks Signed-off-by: Ludovic Orban <lorban@bitronix.be>
…menting async cancelable callbacks Signed-off-by: Ludovic Orban <lorban@bitronix.be>
|
@lorban I agree this approach is invasive... and I have yet to work it all the way through to even show that it is feasible. But I already don't think your proposal in #11860 is feasible (see comments there). What I'd like to do is for us to collaborate on this PR until it is either working... or has discovered all the issues that we don't know about yet from the 20,000 foot view. Specifically, could you have a go at updating the HttpOutput callbacks so that they separate out failure from completion? I'm not entirely sure that can be done with this proposal... or that the contract is exactly correct if it is. Once we have succeeded in making this PR work (or found the killer issue that prevents it from working), then we will be in a position to evaluate if it really is the way to go... brain storm alternatives... look for simplifications.. etc. |
|
@gregw As a summary, we are in agreement that the problem is that we need a formal way to asynchronously cancel callbacks, and that every place where such async cancelling happens should be modified. What we're unsure about is the way to implement that, as I'm going to scavenge through the code base to try to identify all the places where an async cancellation is needed and modify them to separate failure from completion. |
|
@lorban @sbordet I've closed #11857 as a dead end, as my second attempt also got bogged down. However, I am going to have another go as I think there is still some hope. I think Let's hangout tonight (my time) and discuss |
github-project-automation
bot
moved this from 🏗 In progress
to ✅ Done
in 🧊 Jetty 12.0.12 - FROZEN
Jetty version(s)
12.0.9
Description
If an HTTP2 stream gets interrupted by an async failure (like a RESET frame), there is a possibility that the connection gets aborted abruptly.
The cause of this problem is that if the server is busy writing some byte buffers to the socket while
HttpChannel.onFailure()is called by a different thread, the callback passed toChannelResponse.write()is failed, releasing the byte buffers back to the pool after resetting them while theSocketChannel.write()is still running. Upon returning, the NIO call fails withIllegalArgumentExceptionwhile trying to update thelimit, like so:or
Since that exception is thrown from the lowest layer, it is the
HTTP2Flusherthat catches it and its only option is to close the TCP connection as it cannot differentiate between streams.The text was updated successfully, but these errors were encountered: