Configurable chunk size for chunked send file - #6159
Conversation
018f879 to
220a748
Compare
jnbdz
left a comment
There was a problem hiding this comment.
Took a look at this while going through the sendFile-related tickets (I have #5025 in progress on the same files, see the last point). The plumbing is correct as far as I can tell (ChunkedNioFile / UncloseableChunkedNioFile / AsyncFile.setReadBufferSize / VertxConnection.sendFile all receive the value, existing behaviour is unchanged by default). A few remarks, mostly about API shape:
API surface – 9 new overloads on HttpServerResponse for a single int. Two possible ways to keep that smaller, for @vietj to weigh in on:
- put
offset/lengthintoSendFileOptionstoo, so onlysendFile(String, SendFileOptions),sendFile(FileChannel, SendFileOptions)andsendFile(RandomAccessFile, SendFileOptions)are added (3 methods instead of 9), or - make it a server-wide default (
HttpServerOptions.setSendFileChunkSize(int)), which matches the motivation of the issue ("the fallback default is conservative") without any new response API; a per-call variant could still come later.
Layering – io.vertx.core.net.impl.VertxConnection now imports io.vertx.core.http.SendFileOptions only for DEFAULT_CHUNK_SIZE, so net.impl depends on http. A constant in net.impl (with SendFileOptions.DEFAULT_CHUNK_SIZE referencing it) avoids that. UncloseableChunkedNioFile also has an unused import of SendFileOptions left over.
Default methods drop the options – the interface defaults sendFile(..., SendFileOptions) delegate to the variant without options, so a third-party implementation silently ignores them. Both core implementations override, so no issue in core, but the javadoc could say that.
Tests – testSendFileWithOptions / testSendFileRangeWithOptions in the HTTP/1 plain subclass go through the zero-copy path where the option is ignored by design; in the TLS / HTTP/2 subclasses the fallback is used but nothing observes the chunk size. One assertion that the value actually reaches the pump (e.g. the chunkSize=4 range test checking that the body arrives in more than one DATA frame / read, or checking AsyncFile read buffer size for the non-sendfile path) would make the tests meaningful.
Docs – no http.adoc paragraph / HttpExamples snippet for the new option.
Minor – SendFileOptions has no equals/hashCode/toString; NetSocket.sendFile keeps the hard-coded 8 KiB (parity, possibly out of scope).
Overlap heads-up – #5025 (writeFile, i.e. sendFile without ending the response) is being implemented on the same methods and uses UncloseableChunkedNioFile / VertxConnection.sendFile too. If a SendFileOptions object lands, writeFile should take the same object, which is one more reason to keep the overload count low (offset/length inside the options, or a server-level default). Whichever lands first, I'm fine rebasing the other.
|
Hi @jnbdz Thank you for review. I will have detailed view once I have time. |
|
at the moment I think we should have instead of a new options object, a configuration for all send files on the HttpServerConfig object |
Signed-off-by: doxlik <doxlikx@gmail.com>
220a748 to
898015c
Compare
|
@vietj please have a look now |
Currently for non-sendfile (non zero copy) send files the chunk size is hardcoded to 8kb which is good default but not always best size, as example in my benchmarks 32 kb showed much better throughput.
Closes #6160