-
Notifications
You must be signed in to change notification settings - Fork 87
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
stuartwdouglas
wants to merge
1
commit into
jakartaee:master
Choose a base branch
from
stuartwdouglas:connection-reset
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
28 changes: 28 additions & 0 deletions
28
api/src/main/java/jakarta/servlet/ConnectionFailureDetails.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
22
api/src/main/java/jakarta/servlet/ConnectionFailureListener.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
} | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
, notServletRequest
.There was a problem hiding this comment.
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:
I'm good with 2. or 3.
There was a problem hiding this comment.
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.