Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make wait_for_first_flush configurable in streaming responses #66

Merged

Conversation

anmonteiro
Copy link
Contributor

This is related to #65. I managed to get websockets working using
Conduit in order to gain access to the file descriptors. However,
because streaming responses wait for the first write to flush response
bodies, I could never deliver the connection upgrade with 1. an empty
response body and 2. while not closing the writer.

Making wait_for_first_flush configurable (which I see was already in
the roadmap) solves this problem.

I've tested this in my code and it's working really well now. Please
let me know if you had thought of another way of configuring this
behavior.

@anmonteiro anmonteiro force-pushed the anmonteiro/wait-for-first-flush branch 2 times, most recently from 403b225 to a3a6897 Compare August 10, 2018 00:07
@anmonteiro
Copy link
Contributor Author

The Alpine tests are failing for some Docker reasons unrelated to the changes in this PR.

@anmonteiro
Copy link
Contributor Author

An alternative to this PR would be to add a ~flush labeled argument to Response.create and check for it in unsafe_respond_with_streaming.

This is what Cohttp does, for example:
https://github.com/mirage/ocaml-cohttp/blob/b50992f67fffb07a7ec6c0797c2c8cc805bd8011/cohttp/src/s.ml#L119

@sgrove
Copy link
Contributor

sgrove commented Oct 19, 2018

Any progress here @seliopou? Would like to be able to try this out in master.

@seliopou
Copy link
Member

That name's fine for use internally but not great in conveying what the option does to the user. Maybe something more like flush_headers?

Also, it looks like the build's failing due to alpine, which was removed from the .travis.yml in #70 along with an upgrade to OPAM2. Could you rebase past that?

@anmonteiro
Copy link
Contributor Author

@seliopou flush_headers sounds good to me. Otherwise are you willing to accept this change provided I fix those suggestions?

@seliopou
Copy link
Member

Yep.

Wait, how about flush_headers_immediately? I'm open to suggestions I just don't like the current name.

@anmonteiro
Copy link
Contributor Author

@seliopou I was actually gonna suggest either flush_headers_immediately or flush_immediately, but I'm not sure any of these convey the exact meaning of the flag.

@seliopou
Copy link
Member

I think either one improves on the current name. It's not really flushing anything but it is ensuring that the next write will produce data to send. flush_headers_immediately isn't entirely correct since more than the headers will be presenting, but I slightly prefer it to flush_immediately since it is hinting at the part of the message that's getting flushed: namely, everything before the message body. So let's go with that. It can always be changed later if necessary.

Antonio Nuno Monteiro added 2 commits November 17, 2018 16:51
This is related to #65. I managed to get websockets working using
Conduit in order to gain access to the file descriptors. However,
because streaming responses wait for the first write to flush response
bodies, I could never deliver the connection upgrade with 1. an empty
response body and 2. while not closing the writer.

Making `wait_for_first_flush` configurable (which I see was already in
the roadmap) solves this problem.

I've tested this in my code and it's working really well now. Please
let me know if you had thought of another way of configuring this
behavior.
@anmonteiro anmonteiro force-pushed the anmonteiro/wait-for-first-flush branch from a3a6897 to e22c6bb Compare November 17, 2018 16:57
@anmonteiro
Copy link
Contributor Author

@seliopou thanks for your comments. I made the suggested changes and rebased on master.

@anmonteiro
Copy link
Contributor Author

So turns out that renaming the param also involved changing the boolean value, and I forgot to do that. My latest commit does exactly that and also adds a test for this behavior because I don't trust myself anymore.

@seliopou
Copy link
Member

Looks good, thanks!

@seliopou seliopou merged commit 4c37916 into inhabitedtype:master Nov 25, 2018
@anmonteiro anmonteiro deleted the anmonteiro/wait-for-first-flush branch December 8, 2018 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants