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

Surface write failures through [flush] #217

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dhouse-js
Copy link
Contributor

Currently, if you are writing to a body in a streaming manner, and the writer becomes closed, then you have to be careful to not write any more to the body. In practice this can be hard to achieve because your application-level code is probably the one writing to the body and your runtime / network-level code is the one that calls report_write_result `Closed.

This PR therefore makes it safe to write to a body after the attached writer is closed. Those writes are simply ignored.

The user has two ways of discovering whether their writes are actually going to be processed or not:

  • They can look at Body.is_closed, which now returns true if we've detected that the attached writer is closed, in addition to returning true if Body.closed has been called.
  • The type of Body.flush has been augmented so that the callback is told whether the bytes successfully made it out of the writer, or were dropped on the floor. I.e. it has type val Body.flush : Body.t -> ([ `Closed | `Written ] -> unit) -> unit.

ada2k pushed a commit to ada2k/ocaml-h1 that referenced this pull request Jul 31, 2024
Completes merge of inhabitedtype/httpaf#217
Co-authored-by: David House <dhouse@janestreet.com>
Co-authored-by: Doug Patti <douglas.patti@gmail.com>
ada2k pushed a commit to ada2k/ocaml-h1 that referenced this pull request Jul 31, 2024
Completes merge of inhabitedtype/httpaf#217

Co-authored-by: David House <dhouse@janestreet.com>
Co-authored-by: Doug Patti <douglas.patti@gmail.com>
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.

2 participants