Fixes #11072 - Jetty 12: CompleteCallbackHandler #11786
Conversation
CallbackCompletionHandler is a troubleshooting Handler that helps to identify those cases where the Handler/Request/Response APIs are used improperly. In particular, it tracks the events described in CallbackCompletionHandler.Listener, such as the Handler callback not completed, or blocking demand callback, or a write callback not completed, etc. It also provides dump() capabilities, so the current requests and their state is dumped to help troubleshooting. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
There was a problem hiding this comment.
I think at the very least we need to test:
- demand calls made within a demand callback
- demand calls made within a write succeeded callback
- write calls made within a demand callback
- write calls made within a write succeeded callback
It would be interesting to see how those look in a dump when:
- the inner demand/write is pending and the outer callback has not returned
- the inner demand/write able to complete, but the outer callback has not returned
| if (lockedCompleteCallback()) | ||
| return; | ||
|
|
There was a problem hiding this comment.
Can you explain this change? Looks like you are fixing some unrelated issue?
It seams wrong to take action before checking for pending demand and write calls?
The boolean on that method also feels the wrong way around.
There was a problem hiding this comment.
The old code produces an NPE (as httpChannelState is null) when an application returns false from handle(), and then mistakenly completes the callback (which, at that point, is already completed by Jetty producing a 404).
BY doing the early check on whether the callback is already completed, we avoid the NPE.
...e/jetty-server/src/main/java/org/eclipse/jetty/server/handler/CallbackCompletionHandler.java
Outdated
Show resolved
Hide resolved
...e/jetty-server/src/main/java/org/eclipse/jetty/server/handler/CallbackCompletionHandler.java
Outdated
Show resolved
Hide resolved
...e/jetty-server/src/main/java/org/eclipse/jetty/server/handler/CallbackCompletionHandler.java
Outdated
Show resolved
Hide resolved
...e/jetty-server/src/main/java/org/eclipse/jetty/server/handler/CallbackCompletionHandler.java
Outdated
Show resolved
Hide resolved
|
@gregw I have written tests for demand within demand and write within write, because now they are implemented as queues. The other cases seem "normal" ones that are covered in behavior by other tests. I don't think we should test the dump output too much, as likely the format will change; I need a little more convincing that shows it is a good idea to test dump format output 😄 |
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
...e/jetty-server/src/main/java/org/eclipse/jetty/server/handler/CallbackCompletionHandler.java
Outdated
Show resolved
Hide resolved
...e/jetty-server/src/main/java/org/eclipse/jetty/server/handler/CallbackCompletionHandler.java
Outdated
Show resolved
Hide resolved
...e/jetty-server/src/main/java/org/eclipse/jetty/server/handler/CallbackCompletionHandler.java
Outdated
Show resolved
Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java
Show resolved
Hide resolved
Added handling of exceptions thrown by handle(). Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet
deleted the
fix/jetty-12/11072/introduce-callback-completion-handler
branch
Introduced CallbackCompletionHandler.
CallbackCompletionHandler is a troubleshooting Handler that helps to identify those cases where the Handler/Request/Response APIs are used improperly.
In particular, it tracks the events described in CallbackCompletionHandler.Listener, such as the Handler callback not completed, or blocking demand callback, or a write callback not completed, etc.
It also provides dump() capabilities, so the current requests and their state is dumped to help troubleshooting.