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

Add connection failure listener #435

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions api/src/main/java/jakarta/servlet/ConnectionFailureDetails.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package jakarta.servlet;

import java.io.IOException;
import java.util.Optional;

/**
* An interface that carries information about why a connection has failed.
*
* @since Servlet 6.0
*/
public interface ConnectionFailureDetails {

/**
* Returns an optional cause of the underlying failure. If the failure was caused by a protocol level mechanism rather
* than a failure then this may be missing.
*
* @return The cause of the failure
*/
Optional<IOException> cause();

/**
* If this is {@code true} the stream was explicitly reset by the remote endpoint, if this is false then there has been
* a connection level failure rather than the client simply closing this stream.
*
* @return {@code true} if the stream was explicitly reset by the remote endpoint
*/
boolean isStreamReset();
}
22 changes: 22 additions & 0 deletions api/src/main/java/jakarta/servlet/ConnectionFailureListener.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package jakarta.servlet;

import java.util.EventListener;

/**
* A listener that allows an application to be notified of a problem with the underlying connection.
*
* @since Servlet 6.0
*/
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);
}
Comment on lines +10 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

14 changes: 14 additions & 0 deletions api/src/main/java/jakarta/servlet/ServletRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -609,4 +609,18 @@ public AsyncContext startAsync(ServletRequest servletRequest, ServletResponse se
* @since Servlet 6.0
*/
ServletConnection getServletConnection();

/**
* Adds a listener to be eagerly notified of any underlying connection failure. Delivery to this listener is on a best
* effort basis, depending on the implementation and the underlying protocol in use some containers may never be able to
* warn of underlying connection failure.
*
* This listener is intended to allow long running tasks to potentially be cancelled if the underlying connection has
* gone away. Failures will still be reported at the IO level if an attempt is made to read or write to the request or
* response, and if async IO is in use failures may also be delivered to these listeners.
*
* @since Servlet 6.0
* @param listener The listener to be notified.
*/
void addConnectionFailureListener(ConnectionFailureListener listener);
}
16 changes: 16 additions & 0 deletions api/src/main/java/jakarta/servlet/ServletRequestWrapper.java
Original file line number Diff line number Diff line change
Expand Up @@ -515,4 +515,20 @@ public String getProtocolRequestId() {
public ServletConnection getServletConnection() {
return request.getServletConnection();
}

/**
* Adds a listener to be eagerly notified of any underlying connection failure. Delivery to this listener is on a best
* effort basis, depending on the implementation and the underlying protocol in use some containers may never be able to
* warn of underlying connection failure.
*
* This listener is intended to allow long running tasks to potentially be cancelled if the underlying connection has
* gone away. Failures will still be reported at the IO level if an attempt is made to read or write to the request or
* response, and if async IO is in use failures may also be delivered to these listeners.
*
* @param listener The listener to be notified.
*/
@Override
public void addConnectionFailureListener(ConnectionFailureListener listener) {
request.addConnectionFailureListener(listener);
}
}
5 changes: 5 additions & 0 deletions api/src/test/java/jakarta/servlet/MockServletRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -226,4 +226,9 @@ public String getProtocolRequestId() {
public ServletConnection getServletConnection() {
return null;
}

@Override
public void addConnectionFailureListener(ConnectionFailureListener listener) {

}
}