Issue #3806 - Make Async sendError fully Async

[gregw]

This PR for #3806 builds on #3850 with even more "adventurous" improvements.
HttpChannelState in particular has been significantly refactored (hopefully simplified) so that sendError, close and final flushes can be done asynchronously

[sbordet]

@janbartel has pointed out that we have the following pattern in a number of places...

But is not that pattern already racy for the async case? By the time you arrive at the finally block, an application spawned thread could be calling AsyncContext.complete() so that now there is a race with request.isAsyncStarted() and possibly throwing an IllegalStateException when calling request.getAsyncContext().

If the handling throws an exception, we will first go to the finally block and then we will do the ERROR dispatch, so don't think it's a problem.

If the handling calls sendError() we will just change the state and only when we exit the finally block we would do the ERROR dispatch, no?

I think the sync case is ok, it's the async case that is racy.

[sbordet]

@gregw so I have doubts about the broken pattern that you found.
As I see more commits after the joined review we did, should we re-review it? Or more commits are coming?

[gregw]

@sbordet happy to do another review of this.... but the pattern was not racy....

Only a dispatched thread can call startAsync so that is not a race.
Once async, another thread might call complete at any time, but that complete call does not take affect until after the original dispatch has returned. Thus the dispatch thread is still able to add listeners to the AsyncContext (otherwise all listeners would be in a race).

However, the new blocking sendError semantic can be a problem. If an exception is thrown, then we depart via the finally block and do the completeSomething(), in the case of sessions, that might reduce the reference count on the session to 0, so the session is passivated and made non resident. An error page dispatch is then done and the session has to be retrieved again.... but it will now the SessionHandler that used to think that ERROR dispatches could not be for non-resident sessions has to be aware that the session might not be resident and load it, inc reference count etc.

Note that we do have HttpChannel.Listener (why is that not an EventListener) which does give us a good completion callback that works for async and blocking, but that listener is a bit heavy weight and I'd really rather prefer that we didn't use it for something like sessions. I think @janbartel PR is now going to avoid this pattern entirely by having a dedicated completion callback to the request to deal with any session touched by that request. So I think we will be good for sessions (and this PR doesn't break anything that wasn't already a little broken). So I just think it is something we need to be aware of.
Using HttpChannel.Listener in test harnesses to wait for completion is a good way to avoid this pattern for non production code....

Generally speaking... I'm wondering if there is a clever MethodHandle mechanism we could use to plug in listeners to specific callback points without the cost of a list and iteration for every possible callback point, even if we only want 1 method called back.....

[gregw]

I think we need to merge this soon to avoid it going stale.

[sbordet]

This getter/setter pair should be renamed to getCloseCallback() (without d), as they are the callback for method close(), and the field also is without d.

Furthermore, I don't like them. I would prefer to see a new close(Callback) method, so that close() will call close(BLOCKING_CLOSE_CALLBACK), etc.

[gregw]

I have renamed, but can't do close(Callback) at this level as we need to be able to have close passed through a Writer chain as well :(

[sbordet]

I don't understand this code.
It sets the close callback to a non-null one, then calls _response.closeOutput() which calls HttpOutput.close(), which nulls out _closeCallback immediately. So it's always the case that the close callback is null.

On the same spirit of the previous comment, I would call here response.closeOutput(Callback.from(_state::completed)) rather than doing the setter/getter dance.

[sbordet]

What's the purpose of this?
ReservedThreadExecutor may accept STOP tasks, but the problem is that few lines above _tryExecutor has been replaced with NO_TRY so isn't this just a no-op?

[joakime]

I have no additional changes requested over what @sbordet is asking for.
Other then that, I'm good.

[sbordet]

Sorry, still not happy about the close() method.

[sbordet]

Uhm. Now that you have a close(Callback) method, this method can be implemented in terms of the other.

public void close()
{
    FutureCallback callback = new FutureCallback();
    close(callback);
    callback.await();
}

The other method does all the lifting.

My point being that we should avoid having the _closeCallback field and continuously test for it being non-null and completing it. It should just be a local variable in the blocking version of the close() method.
}

[gregw]

@sbordet sorry but it can't be done this way. The new close method is passed a closeable, which either be null or the Writer used for the output. The writer is closed first and if all goes OK, it will be the writer that calls HttpOutput.close(), so there is no possibility to pass the callback through the writer. Thus a field is really the simplest way to capture this.

Simone is on vacation and his last remaining issue cannot be resolved in the way he would like.

[vickumar1981]

is this fix in a snapshot release?

[joakime]

@vickumar1981 it will be available in the upcoming 9.4.21 release.
It is also available as a 9.4.21-SNAPSHOT at the official Jetty snapshot repository.

<repositories>
	<repository>
		<id>jetty.snapshot</id>
		<url>https://oss.sonatype.org/content/repositories/jetty-snapshots</url>
		<releases>
			<enabled>false</enabled>
		</releases>
		<snapshots>
			<enabled>true</enabled>
		</snapshots>
	</repository>
</repositories>