Fix Content.Source reads from within naked threads
#12143
Conversation
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
lorban
changed the base branch from
jetty-12.0.x
to
fix/jetty-12.0.x/12122/httpreceiver-npe
lorban
changed the base branch from
fix/jetty-12.0.x/12122/httpreceiver-npe
to
jetty-12.0.x
jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/thread/SerializedInvoker.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ludovic Orban <[email protected]>
…tty-12.0.x/serialized-invoker-serialized-assertions
lorban
changed the title
SerializedInvoker's serviceability
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
lorban
changed the title
SerializedInvoker's serviceabilityContent.Source reads from within naked threads
…oking Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
…ty to dump them Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
…ty to dump them Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
…ed-invoker-serialized-assertions' into experiment/jetty-12.0.x/serialized-invoker-serialized-assertions # Conflicts: # jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpReceiver.java
…tty-12.0.x/serialized-invoker-serialized-assertions
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
Signed-off-by: Ludovic Orban <[email protected]>
| public SerializedInvoker(Class<?> clazz) | ||
| { | ||
| this(clazz.getSimpleName()); | ||
| } |
There was a problem hiding this comment.
| public SerializedInvoker(Class<?> clazz) | |
| { | |
| this(clazz.getSimpleName()); | |
| } | |
| public SerializedInvoker(Class<?> nameFrom) | |
| { | |
| this(nameFrom.getSimpleName()); | |
| } |
There was a problem hiding this comment.
Done, and added javadoc.
There was a problem hiding this comment.
Somehow this got lost? Can you redo?
…tty-12.0.x/serialized-invoker-serialized-assertions
lorban
force-pushed
the
experiment/jetty-12.0.x/serialized-invoker-serialized-assertions
branch
from
bd23b7f
to
7d98c47
Compare
…tty-12.0.x/serialized-invoker-serialized-assertions
…tty-12.0.x/serialized-invoker-serialized-assertions
|
@lorban why force push????? I now can't see what you have changed since my last review. I'm putting this to the back of my re-review queue as "punishment"!!!!! |
|
This happened because I mistakenly merged in 12.1.x instead of 12.0.x, and re-merging 12.0.x afterwards gave me a million conflicts, so it was easier to reset the history to before the 12.1.x merge and force-push that. Sorry about the inconvenience. |
|
Oh I did the same to Simone's PR! |
...y-client/src/main/java/org/eclipse/jetty/client/transport/internal/HttpReceiverOverHTTP.java
Show resolved
Hide resolved
| public SerializedInvoker(Class<?> clazz) | ||
| { | ||
| this(clazz.getSimpleName()); | ||
| } |
There was a problem hiding this comment.
Somehow this got lost? Can you redo?
Signed-off-by: Ludovic Orban <[email protected]>
|
@gregw this PR was used to identify a problem, its cause and to propose and quick and dirty fix, all that being concentrated in responseContentAvailable(HttpExchange). The A much cleaner fix is being worked on in #12203 which is going to replace this PR. |
|
@lorban so this is not going to be merged? |
|
Superseded by #12203 |
We have methods implicitly assuming that they're only ever going to be called from an invoker. That's a bit of a leap of faith as there currently is no way to check that and that's not always documented. Let's add some code that allows asserting that the execution is happening from the invoker, akin to when we assert that a lock is held by calling
AutoLock.isHeldByCurrentThread().Adding such extra assertions surfaced a bug introduced by #12123, as calling
Content.Source.read()out of the demand's runnable callback would callHttpReceiver.responseContentAvailable()outside the invoker's context, so modifications are needed to make sure that the invoker is always used when needed.Let's also name the SerializedInvoker instances to improve readability when they are logged.