- Notifications
You must be signed in to change notification settings - Fork137
Support for unbuffered writes#229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hey@nightmared thanks so much for planning and building this draft.
First of all to give you some confidence to continue, I'd be happy to merge this feature, I think it's a useful extension on the current library behaviour.
If you could include some of your commentary on the change in the commit message when you finalise the PR that would be great too.
Uh oh!
There was an error while loading.Please reload this page.
2355e58
to3a4ed4f
CompareUh oh!
There was an error while loading.Please reload this page.
3a4ed4f
to4968af3
CompareAlright, I have reworked the PR to rebase it on master, and also:
Note that the rustc-1.56 failure is not due to the PR itself, but the need to bump the minimal version in the CI to 1.57 for rustls. |
0197e28
to22db82a
CompareThis is achieved through two different means:- The ability to disable buffering inside `Response` objects with the `Response::with_buffering` method. Enabling buffering will force the transfer encoding to be `TransferEncoding::Chunked` and configure the chunks encoder to flush to its underlying writer on every write.- To get "instantaneous" write, disabling buffering in the chunks encoder is not enough, as the underlying writer returned when calling `Server::recv()` (`ClientConnection.sink`) is in fact a `BufWriter` wrapping the "real" output. The `writer_buffering` parameter in `ServerConfig.advanced` can alter the server behavior to omit the BufWriter when writing to the TcpStream.This will probably decrease performance significantly when sending big files,which is why these two subfeatures are disabled by default, and must be opted-in(by calling the `with_buffering` method for the first, and by instanciating theserver with a call to `with_writer_buffering_mode` for the second).
22db82a
to7ca33d0
Compare@bradfier Sorry to dust up this old PR, but there were conflicts preventing an eventual merge, so I updated the PR and reworked the then-outdated commit message. |
Hello,
this PR aims to tackle one problem I had: streaming the output of a program while it is running over HTTP, similarly to the Github/GitLab CI "live" screens.
In fact, the goal is to perform a request to the server with cURL inside a gitlab-ci job, and see the output unfolds as the program is executed:
Without this patch, the program executes in its entirety, and the output is sent all at once
once the pipe where the program writes its output is closed.
To achieve that goal, I submit two "sub-features":
Response
objects with theResponse::with_buffered
method.Enabling this will force the transfer encoding to be
TransferEncoding::Chunked
and will ask thechunks encoder to flush to its underlying writer on every write.
writer returned when calling
Server::recv()
(ClientConnection.sink
) is in fact a BufWriter wrappingthe "real" output. The
buffered_writer
option inServerConfig
, when set to false while instantiatinga new server, omits the BufWriter to write to the TcpStream withotu any buffering. The cost of that
abstraction is that
ClientConnection.sink
now boxes the writer to be able to choose betweenBufWriter<RefinedTcpStream>
orRefinedTcpStream
dynamically, which means there is now one additionalpointer deference. I do however expect the performance impact to be small as this pointer is then stored
in an
Arc<Mutex<>>
, and I think locking/unlocking the mutex should be more costly that deferencing thepointer.
I expect this to decrease performance when sending big files, which is why these two sub-features are disabled
by default, and must explicitly be opted-in (by calling the
with_buffered
method for the first, and byinstanciating the server with the
buffered_writer
setting for the second).Also note that the current iteration of this work breaks the current ABI, as it updates
ServerConfig
toadd the
buffered_writer
option, andServerConfig
was already exposed through theServer::new
function.