HttpInput may skip setting fill interest #5692
Conversation
Code cleanups and logging improvements. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
HttpInput.run() now uses contentProvider.isReady() to ensure that if there is no content, the fill interest is set. AsyncContentProvider.isReady() is now looping if there is content but it cannot be transformed (e.g. too few gzipped bytes). Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
| @@ -222,24 +222,39 @@ public boolean isReady() | |||
| if (content == null) | |||
| { | |||
| _httpChannel.getState().onReadUnready(); | |||
| if (_httpChannel.needContent()) | |||
| while (true) | |||
There was a problem hiding this comment.
I think this method is correct, but I think it would read better as:
public boolean isReady()
{
HttpInput.Content content = nextTransformedContent();
if (content != null)
{
// TODO do we really need to call onContentAdded in this case??? I don't think so as we have not called onReadUnready
return true;
}
_httpChannel.getState().onReadUnready();
while (_httpChannel.needContent())
{
HttpInput.Content content = nextTransformedContent();
if (content != null)
{
_httpChannel.getState().onContentAdded();
return true;
}
_httpChannel.getState().onReadUnready();
}
return false;
}
There was a problem hiding this comment.
@gregw Regarding your TODO, yes you must call onContentAdded in this case. Imagine a servlet calling isReady twice. The first time false is returned but the 2nd time true is returned. If you do not call onContentAdded to switch the state to READY, it'll be stuck at UNREADY which will cause havoc.
Oh, and I also added in the javadoc of ContentProducer.isReady that After this call, state can be either of UNREADY or READY as the state cannot be left IDLE after isReady is called.
There was a problem hiding this comment.
@lorban But if isReady() has returned false, then regardless of it being called another time there is a scheduling action that will take place behind the scenes to make sure onDataAvailable is called when data becomes available. Surely it is that action that will call onContentAdded. Can you write a test case to demonstrate why this call is needed?
There was a problem hiding this comment.
I've opened #5704 to track this.
Updates after review. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java
Show resolved
Hide resolved
jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java
Show resolved
Hide resolved
joakime
changed the title
@gregw @lorban the first commit is logging improvements, the second commit is the actual fix.