Undisable ee9 BlockingTest and fix HttpChannel.produceContent #12529
Conversation
| ContextHandler.CoreContextRequest coreContextRequest = getCoreRequest(); | ||
| if (coreContextRequest == null) | ||
| return null; |
There was a problem hiding this comment.
coreContextRequest can only be null after recycle(), so returning null in this case is wrong as that implies a call to needContent() should follow.
I think returning an new ErrorContent(new IllegalStateException("Illegal read from complete request")) would be the right thing to do when coreContextRequest is null.
There was a problem hiding this comment.
I don't see the difference between returning null here, and returning null at line 179 when there is no chunk?
There was a problem hiding this comment.
produceContent() returning null has a special meaning: that means there's no content available right away, but more will follow, i.e.: everything is normal, there's just no content to be read right away. Error cases OTOH should always be returned in the form of an ErrorContent object.
When coreContextRequest is null, this means the channel has been recycled, which is an error case.
By the way, this coreContextRequest null check should also be done in needContent() as both needContent() and produceContent() fulfill the same contract about the availability of content.
There was a problem hiding this comment.
@lorban it's not possible to return ErrorContent for needContent(): you can return false or throw an Exception, which would you prefer?
There was a problem hiding this comment.
@janbartel as part of my commit, I changed needContent() to make it return true when coreContextRequest is null.
Signed-off-by: Ludovic Orban <[email protected]>
HttpChannel.produceContent throws NPE if the core request has already been finished when it is called. This caused some of the BlockingTests to fail.