Issue #6167 - Review failure cases of HttpOutput.Interceptor

[lachlan-roberts]

Issue #6167

This PR adds extra testing around what happens if an HttpOutput.Interceptor throws or fails the callback.

Some changes were made to HttpOutput so that in most cases we will respond with a 500 response instead of just aborting the connection.

[gregw]

Well I like the approach.... other than my niggles about test, name and javadoc.
But I'd still like to hear from @sbordet regarding throwing from async close/write/flush.

[sbordet]

This piece of code after channelWrite() is in a race with the WriteCompleteCB, so I think you should only do the work in WriteCompleteCB.

[lachlan-roberts]

This code is about getting the close() method to throw if the WriteCompleteCB has been failed before we get to this (like if the interceptor failed it). We do this here to report the exception back to the application without aborting the channel so we can send a 500 response.

[sbordet]

addInterceptor() seems unnecessary. Why don't you just add the interceptor from the AbstractHandler that you have in every test? In this way you also won't need _handlerCollection.

Something like:

@Test
public void testInterceptorCallbackFailed() 
{
    start(new AbstractHandler()
    {
        @Override
        public void handle(...) 
        {
            HttpOutput httpOutput = baseRequest.getResponse().getHttpOutput();
            httpOutput.setInterceptor(...);
            ...
        }
    });
}

[lachlan-roberts]

Not all the interceptors added by the tests do the same thing, some fail the callback, some throw, some start a new thread and wait on a CountDownLatch before failing the callback.

[sbordet]

@lachlan-roberts you can do what you need with the interceptor on the second line of the handler, no?

[gregw]

I think we should be more explicit that this code is in a known race, even though it will work the same:

Suggested change

	if (_onError != null)
	IO.throwCheckedIOException(_onError);
	Throwable onError = _onError; // Was an exception reported already
	if (onError != null)
	IO.throwCheckedIOException(onError);

@sbordet I told @lachlan-roberts that you wouldn't like this code... yes it is a race for a general type of async exception, but there is a class of exception that will be thrown during the processing of the close, and it is those that we should throw here so the application can know all was not OK.

[sbordet]

@gregw @lachlan-roberts IIUC this if block is the case where we do an async close, which is actually our educated interpretation of what we should do in case of async I/O, because it's underspecified.
I'm not sure it's worth reporting it with an exception, because we already report it with onError() (and if we don't we probably should).

I am worried about the case where we throw from close, but also fail the callback, and we will have one thread receiving the exception trying to write the error page, and to do that modify the channel and output states, and another thread failing the callback also modifying the channel and output states, and calling onError().
I feel that we are just asking for troubles, garbled responses, etc.

If there is an exception in an async close, we call onError() and that's enough notifications to the application.

[lachlan-roberts]

The problem is that we don't call onError() if there is a failure with the async close.
If there is the following code:

httpOutput.close();
asyncContext.complete();
return;

then you can't call onError() because asyncContext.complete() was called and there was no call to isReady() which returned false.

[gregw]

Moreover, we can't rely on calling onError for issues at the end of the stream, because when we have an exception or are at end of stream, then isReady() returns true, so the app will loop and not return from onWritePossible, so we will never be able to call onError even if we wanted to!

[sbordet]

If we don't call onError(), which is the usual path for errors in async I/O, why do we even bother?

The exception may or may not be thrown depending on thread scheduling etc, so it's not reliable behavior, we have a race in our code and we don't provide any reliable benefit to user code, just that sometimes, under certain conditions, if the thread scheduling aligns, maybe we throw.

This is the kind of code that in 6 months we are going to have an issue reported and ask ourselves "who put that racy code in there?"

I don't think it's worth it.

[gregw]

Because if we don't throw, then we end up aborting the connection rather than sending a 500

[sbordet]

@gregw some times we send a 500; some other times we don't.
If it was possible to always send a 500, would be good -- even if it was racy.
But if we can't always send a 500, then I don't think it's worth it, also considering that calling close() explicitly is not that common.

[gregw]

But we also throw from write or flush, so it's just making it the same as those operations

[sbordet]

But in write() and flush() there is no race.

[sbordet]

@gregw @lachlan-roberts IIUC this if block is the case where we do an async close, which is actually our educated interpretation of what we should do in case of async I/O, because it's underspecified.
I'm not sure it's worth reporting it with an exception, because we already report it with onError() (and if we don't we probably should).

I am worried about the case where we throw from close, but also fail the callback, and we will have one thread receiving the exception trying to write the error page, and to do that modify the channel and output states, and another thread failing the callback also modifying the channel and output states, and calling onError().
I feel that we are just asking for troubles, garbled responses, etc.

If there is an exception in an async close, we call onError() and that's enough notifications to the application.

[gregw]

Considering @lachlan-roberts research regarding this, I am switching this PR back to draft and Lachlan should work on a version that returns false from isReady if there is an exception that will be passed to onError.

@lachlan-roberts can you write up your findings either here on in the actual issue.

[gregw]

Try changing so isReady() returns false if an exception has occurred

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[github-actions]

This pull request has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[github-actions]

This pull request has been closed due to it having no activity.