Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
nightmared wants to merge1 commit intotiny-http:master
base:master
Choose a base branch
Loading
fromnightmared:unbuffering_support_pr

Conversation

nightmared
Copy link

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:

              web browser----------    ------------> -------------------| CLIENT |                  |  GitLab CI job  |----------   <------------  -------------------               streams the     |              ∧                  output       |              |                               | executes     | prints the                               |              | output                               |              | "live"                               ∨              |                             -------------------                             |       cURL      |                             -------------------                              |               ∧                              |               | streaming                              | HTTP          | (chunked                              | query         | transfer)                              |               | of stdin                              |               | and stdout                  spawns      ∨               |-----------  <------------   --------------------| program |                  | tiny-http server |-----------   ------------>  --------------------               sends stdout               and stdin in                 a pipe

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":

  • The ability to disable buffering insideResponse objects with theResponse::with_buffered method.
    Enabling this will force the transfer encoding to beTransferEncoding::Chunked and will ask 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 callingServer::recv() (ClientConnection.sink) is in fact a BufWriter wrapping
    the "real" output. Thebuffered_writer option inServerConfig, when set to false while instantiating
    a new server, omits the BufWriter to write to the TcpStream withotu any buffering. The cost of that
    abstraction is thatClientConnection.sink now boxes the writer to be able to choose between
    BufWriter<RefinedTcpStream> orRefinedTcpStream dynamically, which means there is now one additional
    pointer deference. I do however expect the performance impact to be small as this pointer is then stored
    in anArc<Mutex<>>, and I think locking/unlocking the mutex should be more costly that deferencing the
    pointer.

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 thewith_buffered method for the first, and by
instanciating the server with thebuffered_writer setting for the second).

Also note that the current iteration of this work breaks the current ABI, as it updatesServerConfig to
add thebuffered_writer option, andServerConfig was already exposed through theServer::new function.

clauverjat reacted with thumbs up emoji
Copy link
Member

@bradfierbradfier left a 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.

@nightmared
Copy link
Author

Alright, I have reworked the PR to rebase it on master, and also:

  • removed all the boolean parameters.
  • Added a newMaybeBufferedWriter enum to remove theBox<dyn Write> that I have introduced in the previous version. This should lead to slighter better performance (and mainly produce less allocations).

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.

bradfier reacted with eyes emoji

@nightmarednightmaredforce-pushed theunbuffering_support_pr branch 2 times, most recently from0197e28 to22db82aCompareAugust 21, 2024 18:33
This 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).
@nightmared
Copy link
Author

@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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@bradfierbradfierAwaiting requested review from bradfier

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants
@nightmared@bradfier

[8]ページ先頭

©2009-2025 Movatter.jp