Improve HttpConnection buffer recycling #12227 #12228
Conversation
Improved tests to indicate the issues.
Passing more tests now
Passing more tests now
Passing more tests now
| // We are not in a race here for the request buffer as we have not yet received a request, | ||
| // so there are not any possible legal threads calling #parseContent or #completed. |
There was a problem hiding this comment.
The comment is incorrect.
This method is called from parseAndFillForContent(), which is called by the application either reading or demanding, so a request has indeed been received.
| @@ -411,30 +422,36 @@ public void onFillable() | |||
| if (_handling.compareAndSet(true, false)) | |||
| { | |||
| if (LOG.isDebugEnabled()) | |||
| LOG.debug("request !complete {} {}", request, this); | |||
| LOG.debug("request !complete {} {} {}", request, this, _requestBuffer); | |||
| releaseRequestBufferIfEmpty(); | |||
There was a problem hiding this comment.
This is in a race with the application, so we cannot release at all here.
A thread spawned by the application can read request content, possibly leading to unexpected NPEs.
| releaseRequestBuffer(); | ||
| } | ||
| else if (filled < 0) | ||
| _parser.atEOF(); |
There was a problem hiding this comment.
Cannot remove buffer release from this method, because this method is called from parseAndFillForContent() (which does not release buffers), so reading the request content would never release the buffer.
| // The handling thread has completed, so we can free up any empty buffer here | ||
| releaseRequestBufferIfEmpty(); | ||
|
|
There was a problem hiding this comment.
The idea of not releasing here is that we can re-use an empty existing buffer for the next request, rather than releasing here, and having to re-acquire it for the next request.
Alternative to #12228. In this PR, the responsibility to release the buffers is in 2 methods: onFillable() (called when network data is available, and to process the next request) and parseAndFillForContent() (called from Request.read()). Signed-off-by: Simone Bordet <[email protected]>
Fix #12227 by improving/clarifying buffer recycling
Added more tests to better check the behaviour