Fixes #10277 - Review read failures impacting writes. #10948
Conversation
Separated read failures from write failures. In this way it is possible to read even if the write side is failed and write even if the read side is failed. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
| @@ -1115,78 +1111,76 @@ public void write(boolean last, ByteBuffer content, Callback callback) | |||
| { | |||
| long length = BufferUtil.length(content); | |||
|
|
|||
| HttpChannelState httpChannelState; | |||
| HttpChannelState httpChannel; | |||
There was a problem hiding this comment.
Shorter and used elsewhere as just httpChannel.
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Outdated
Show resolved
Hide resolved
|
Interesting. |
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Show resolved
Hide resolved
Fixed tests. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Show resolved
Hide resolved
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
There was a problem hiding this comment.
@sbordet the changes for the existing write failure are good... but the change to call onFailure appears to be breaking some tests. Can we revert those and try after this release? I have some more error changes that might be needed to handle the AsyncIOServletTests that are currently disabled and that I will be looking at as part of #10933
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
| // Try to fail the request, but we might lose a race. | ||
| try | ||
| { | ||
| request._callback.failed(failure); |
There was a problem hiding this comment.
I've thought about this some more and I'm OK with this failed called, at least until we are sure. In the HandlerInvoker, we do similar if the call to handle throws. This is much the same thing.
There was a problem hiding this comment.
@gregw not sure it's the same thing. If we throw from handle() we documented "equivalent to fail callback", but throwing e.g. from a request listener or from a demand callback or from a write callback does not seem equivalent as leaving handle() with an exception.
There was a problem hiding this comment.
@sbordet I'm not sure either. Your build is green, so OK for this release... but it is a cautious OK.
There was a problem hiding this comment.
@sbordet throwing from a callback that is not meant to throw is a pretty good indication of an application that is not behaving correctly. I wonder at the wisdom of letting it hang forever saying "you should not have thrown and should have called the callback yourself", when we already know it is broken and if they complain that we have failed the callback, then we just say "don't throw!"
There was a problem hiding this comment.
It might be better to always fail the callback in this case?
There was a problem hiding this comment.
I think of it in a different way: it's an unexpected failure and we have a mechanism for that, failure listeners.
Register a failure listener and you will be notified and you can complete the callback (as you can't really do much else).
Here a proposal for a catch-all Handler.
There was a problem hiding this comment.
The catch-all is a good idea regardless. I'm just worried about us being too purist. We know an application that throws from a callback is bad and we know that we will probably hang forever on that request.
I agree that we should never fail the callback if there is a possibility the application is acting correctly, but I'm OK with failing it when we know it is not.
There was a problem hiding this comment.
@sbordet eitherway, let's punt to next release. we have more error work to do anyway.
Separated read failures from write failures.
In this way it is possible to read even if the write side is failed and write even if the read side is failed.