Add connection failure listener

[stuartwdouglas]

This allows applications to react to the client closing the connection
in a timely manner, without needing to perform IO to be notified.

Signed-off-by: Stuart Douglas [email protected]

[gregw]

I'm not opposed to conneciton listeners, but think we'd want to be able to listen for normal as well as abnormal closes.

To avoid races, we would also need to say that any listener that was set after the connection was closed, but before a normal dispatch to the application had returned, would still be called after the dispatch returns.

[gregw]

If we are going to have a connection listener, then we should be able to listen for a normal close.

Suggested change

	public interface ConnectionFailureListener extends EventListener {
	/**
	* This method is invoked on a best effort basis if the underlying connection has gone away.
	*
	* This method will generally be invoked from a different thread to any other threads currently running in the
	* application, so any attempt to stop application processing as a result of this notification should be done in a
	* thread safe manner.
	*
	* @param details Information about the failure
	*/
	void onConnectionFailure(ConnectionFailureDetails details);
	}
	public interface ConnectionListener extends EventListener {
	/**
	* This method is invoked on a best effort basis if the underlying connection has gone away abnormally.
	*
	* This method will generally be invoked from a different thread to any other threads currently running in the
	* application, so any attempt to stop application processing as a result of this notification should be done in a
	* thread safe manner.
	*
	* @param details Information about the failure
	*/
	void onError(ConnectionFailureDetails details);
	/**
	* This method is invoked on a best effort basis if the underlying connection has closed normally.
	*
	* This method will generally be invoked from a different thread to any other threads currently running in the
	* application, so any attempt to stop application processing as a result of this notification should be done in a
	* thread safe manner.
	* @param details Information about the connection
	*/
	void onClosed(ConnectionDetails details);
	}

[stuartwdouglas]

How would that work though? You could have thousands of requests using the same connection, if you needed to hang onto the listener for every one of those requests it represents a huge memory leak. 'closed normally' can pretty much only happen after response processing is finished, and the request is basically done.

[gregw]

I do not see multiple requests creating the same listener over and over.

The use-case for this is apps that are going to use the new connection ID to create state in a map (of caches or authentication etc.)
Currently they can do this already, but have no way to remove items from a map of connection ID to stuff. So we have created a memory leak of sorts already.

So we need a listener to be added if a putIfAbsent succeeds into some kind of map. However, if the connection fails immediately after the putIfAbsent, then the listener might be registered too late and the app will never remove the connection.

Think of it another way, a Connection listener will never be called to say the connection is closed or failed whilst there is an application thread actively dispatched.
This is no different to how we currently don't deliver WL/RL/AL onError calls in the same situation.

[gregw]

Note a fix for the tooooo many listener thing could be to only have a setter rather than an adder for the connection listener. Then there could only be 1 per connection. I guess there could then be a problem with multiple concerns all wanting to clear their own connection caches....

[stuartwdouglas]

My issue is though that the two use cases we are discussing here have very different scopes. The use case 'I want to be able to stop a long running task if the underlying connection is reset' is very different to the 'clear a cache when the connection is closed'.

In the first one the listener is only relevant while the current request is being processed, while the second one is actually scoped to the life of the connection.

[markt-asf]

It is not clear to me when "connection" is being used in this discussion whether "connection" is equivalent to HTTP/2 stream or HTTP/2 connection.

My current view is that the listener should be set at the request (i.e. stream) level. If an HTTP/2 connection failed, that would trigger the listeners (if any) for all current streams for that connection.

[gregw]

@markt-asf No the connection needs to be at the h2 connection level. There are protocols over http (some apple store one for example) that do authentication on the h2 connection in one stream. Once that one stream is authenticated, then all other streams on that connection are considered to be authenticated.

Currently it is imposible to implement that protocol with servlets because we cannot reason about the connection. With the new connection ID, we can now keep that authentication info, but need listeners so that it doesn't become a memory leak.

I don't think we need a new listener for stream issues as if it closes normally, then the request/response cycle is already over. If it fails abnormally then we have WL.onError, RL.onError and/or AL.onError to report it.

@stuartwdouglas there is no reason that long running CPU tasks need to be associated with a single request. I've seen many apps that start a task and then use multiple requests to query how it is going. If the connection to that peer is lost then you have to use timeouts to stop the CPU task, but a connection listener would allow it to be stopped in a timely way. So yes, there are different scopes... probably a spectrum of different scopes. But I think the listener as I proposed can support them all, with the slight risk that if every request sets a new connection listener, but if you allocate any long lived resource on every request you will have problems.

[markt-asf]

@gregw Tx. I now understand that particular use case but what about the stream reset use case? I think you are suggesting that a single request for a long running task is poor design (I agree) and therefore we shouldn't support it. I think I am fine with that.

If the listener is a connection level listener than I think it should be set on ServletConnection, not ServletRequest.

[gregw]

+1 the listener should be added/set on the ServletConnection not the request.

As for the stream reset case, Jetty currently delivers a stream reset to the AL.onError as well as an RL/WL registered. Note that doing so does not support long running threads in previous container managed threads because all these callbacks are mutually excluded. But they do support long running other threads that might run over one long async request or over many requests over 1 connection.

So I think we are pretty much agreed on RL/WL onError - the call it for any IO regardless of isReady state if the corresponding stream is not closed.

I think the only difference now is how we call AL.onError for IOExceptions with the options being:

1. never call it
2. call it only if there is not a RL/WL to deliver the exception to
3. always call it.

I'm good with 2. or 3.

[stuartwdouglas]

I was proposing this as a way to handle RST_STREAM notifications (and potentially other underlying issues that we can detect via background async IO).

The reason why I don't like the AL approach is that is means you can only detect this if you are doing an async request, which seems like a fairly arbitrary limitation.

I do agree that the connection cache based approach is valid for some use cases that unfortunately tie state to a connection, but IMHO they are two separate problems.