Improve handling of idle timeouts when the handling itself is idle #12301
Conversation
Always remember the write failure
gregw
added
the
Sponsored
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java
Outdated
Show resolved
Hide resolved
I don't think this would work, because we need to be able to write a response if an idle timeout or a failure happens. What is the reason behind this change? |
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadPool.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadPool.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadPool.java
Show resolved
Hide resolved
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadPool.java
Outdated
Show resolved
Hide resolved
| @@ -1161,17 +1159,17 @@ private boolean lockedIsWriting() | |||
| return _writeCallback != null; | |||
| } | |||
|
|
|||
| private Runnable lockedFailWrite(Throwable x) | |||
| private Runnable lockedFailWrite(Throwable x, boolean alwaysFailFutureWrites) | |||
There was a problem hiding this comment.
The new parameter is always false, so remove it?
There was a problem hiding this comment.
I'm not entirely convinced that we should never do this. But I will remove for now.
|
@gregw so with the removal of the parameter I don't think failing writes is correct, because we want to be able to write an error response in case of failures or idle timeouts. |
I'm not understanding your comment? This PR is no longer failing writes after a failure. It did for a while, but after discussions we couldn't convince ourselves that it was right to do so. I'm not entirely convinced it is wrong to do so, but keeping the behaviour the same as currently except ensuring the Runnable is executed is a good step. Why is it so important to write a response when there is a failure? Most times when this happens, the response will never make it to the client, as the connection will have failed in some way, hence the failure/timeout. I know we have always strived to do this, but I'm wondering what is the value? The cost is that we can sometimes ignore real errors and not deliver them to an application, which then doesn't see those failures on subsequent writes. Perhaps this PR to improve the invocation of error listeners will avoid the problem, but I'm still not 100% sure. |
…12301) Always execute (without queuing) the task to call error listeners
Currently if a handler is idle (or blocked) when an idle timeout occurs, it is essentially a noop, unless the handler has an idle timeout listener, or subsequently reads from the request.
This change ensures that all subsequent writes will also fail.