Explicitly drain the leftover bytes in buffer for the POST request in HTTPHander#81461
Explicitly drain the leftover bytes in buffer for the POST request in HTTPHander#81461nikitamikhaylov wants to merge 2 commits intomasterfrom
Conversation
|
Kudos to @vdimir for writing the test! |
| { | ||
| String remaining; | ||
| readStringUntilEOF(remaining, *in_post_maybe_compressed); | ||
| auto hex_string = hexString(remaining.data(), remaining.size()); |
There was a problem hiding this comment.
Might want to shorten this to a substring, otherwise it can print a lot e.g. if the query is cancelled with memory limit exceeded.
There was a problem hiding this comment.
More generally, maybe it should only print the data if the query was successful and having data still in the buffer is unexpected?
There was a problem hiding this comment.
IIUC, if query fails executeQuery above throws exception and we don't get here.
| /// 3. Make HTTPChunkedReadBuffer eagerly drain the final \r\n0\r\n\r\n before returning the final bytes of data, | ||
| // but that seems very inconvenient to implement because of how zero-copying is done in HTTPChunkedReadBuffer::nextImpl() | ||
| /// You can find an option (2) implemented below. | ||
| if (in_post_maybe_compressed->available()) |
There was a problem hiding this comment.
available() is data that happens to be already in the buffer. Don't we need to actually read from socket too (including blocking reads), if the expected final bytes weren't received yet? E.g. if the sender sleeps for a second after sending all the data but before sending the final \r\n0\r\n\r\n.
I think we can make HTTPServerRequest constructor set a flag saying whether the buffer needs to be drained (in the getChunkedTransferEncoding() and hasContentLength() cases, but not in the POST-of-unknown-size case since we wouldn't know how much to drain). Then here we'd do readStringUntilEOF if that flag is set.
|
@al13n321 Feel free to close this one, but don't forget to write a test / use the one from the current PR. |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Previously, the error
Invalid HTTP version string: /?query=Imight have occurred while inserting some data over HTTP. This happened because ClickHouse didn't fully consume the input bytes (\r\n0\r\n\r\n) and reused the connection for the subsequent requests. This behaviour is now fixed.Documentation entry for user-facing changes