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.

Sign up for GitHub

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

Jump to bottom

Cohttp_async.Body.is_empty should return true iff pipe is empty #712

Merged
merged 1 commit into from
Sep 9, 2020
Merged

Cohttp_async.Body.is_empty should return true iff pipe is empty #712

merged 1 commit into from
Sep 9, 2020

Conversation

anuragsoni
Copy link
Contributor

Before this change i was seeing the following:

Body.is_empty (`Pipe (Pipe.of_list [""; "foo"])) = true
Body.is_empty (`Pipe (Pipe.of_list [])) = false

Signed-off-by: Anurag Soni anurag@sonianurag.com

@anuragsoni
Cohttp_async.Body.is_empty should return iff pipe is empty
389b70c 
Signed-off-by: Anurag Soni <anurag@sonianurag.com>
@anuragsoni
Copy link
Contributor Author

This should align the behavior with Cohttp_lwt's Lwt_stream handling.

On a related note, I also noticed that cohttp considers String "" as an empty body, but Strings [""], Stream (Lwt_stream.of_list [""]), and now Pipe (Pipe.of_list [""]) will not be considered as empty. I'm wondering if it'd make sense to treat the various forms of list [""] bodies as empty as they don't contain any bytes?

@anuragsoni anuragsoni changed the title Cohttp_async.Body.is_empty should return iff pipe is empty Cohttp_async.Body.is_empty should return true iff pipe is empty Sep 9, 2020
@mseri
Copy link
Collaborator

mseri commented Sep 9, 2020

I agree that it would make sense to consider also empty lists as empty. Would you mind adding the change?

@mseri
Copy link
Collaborator

mseri commented Sep 9, 2020

As an aside, please have a look at #692 Any feedback welcome.

@anuragsoni
Copy link
Contributor Author

I agree that it would make sense to consider also empty lists as empty. Would you mind adding the change?

@mseri I will put up a PR for discussion, but i think it might be better to do that as a separate PR so this commit can be about fixing the bug in the async Body module. What do you think? I will be able to put up a PR later today or tomorrow.

Thanks!

@mseri
Copy link
Collaborator

mseri commented Sep 9, 2020

Sounds good to me

@mseri mseri merged commit 0203382 into mirage:master Sep 9, 2020
@anuragsoni anuragsoni deleted the async-body-pipe-is-empty branch September 9, 2020 13:59
@anuragsoni anuragsoni mentioned this pull request Sep 9, 2020
Closed
mseri added a commit to mseri/opam-repository that referenced this pull request Nov 2, 2020
@mseri
[new release] cohttp-lwt-unix-nossl, cohttp-lwt-jsoo, cohttp, cohttp-…
25a4517 
…lwt, cohttp-lwt-unix, cohttp-lwt-unix-ssl, cohttp-top, cohttp-async and cohttp-mirage (3.0.0)

CHANGES:

- cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711)
- cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707)
- cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715)
- cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706)
- cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713)
- port to conduit 3.0.0: minor breaking changes on the server API, an explicit distinction
  between cohttp-lwt-unix (using tls), cohttp-lwt-notls and cohttp-lwt-ssl (using lwt_ssl),
  and includes ssl in cohttp-async (@dinosaure mirage/ocaml-cohttp#692)
- cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717)
- refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692)
- update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720)
- cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721)
- lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704)
- fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722)
- add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723)
- cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696)
- improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725)
- add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)
@mseri mseri mentioned this pull request Nov 3, 2020
Merged
mseri added a commit to mseri/opam-repository that referenced this pull request Nov 5, 2020
@mseri
[new release] cohttp-lwt-unix-nossl, cohttp-lwt-jsoo, cohttp, cohttp-…
39cc039 
…lwt, cohttp-lwt-unix, cohttp-lwt-unix-ssl, cohttp-top, cohttp-async and cohttp-mirage (3.0.0)

CHANGES:

- cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711)
- cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707)
- cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715)
- cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706)
- cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713)
- port to conduit 3.0.0: minor breaking changes on the server API, an explicit distinction
  between cohttp-lwt-unix (using tls), cohttp-lwt-notls and cohttp-lwt-ssl (using lwt_ssl),
  and includes ssl in cohttp-async (@dinosaure mirage/ocaml-cohttp#692)
- cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717)
- refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692)
- update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720)
- cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721)
- lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704)
- fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722)
- add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723)
- cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696)
- improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725)
- add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)
mseri added a commit to mseri/opam-repository that referenced this pull request Nov 5, 2020
@mseri
[new release] cohttp-lwt-unix-nossl, cohttp-lwt-jsoo, cohttp, cohttp-…
962160d 
…lwt, cohttp-lwt-unix, cohttp-lwt-unix-ssl, cohttp-top, cohttp-async and cohttp-mirage (3.0.0)

CHANGES:

- cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711)
- cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707)
- cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715)
- cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706)
- cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713)
- port to conduit 3.0.0: minor breaking changes on the server API, an explicit distinction
  between cohttp-lwt-unix (using tls), cohttp-lwt-notls and cohttp-lwt-ssl (using lwt_ssl),
  and includes ssl in cohttp-async (@dinosaure mirage/ocaml-cohttp#692)
- cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717)
- refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692)
- update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720)
- cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721)
- lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704)
- fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722)
- add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723)
- cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696)
- improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725)
- add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)
mseri added a commit to mseri/opam-repository that referenced this pull request Nov 5, 2020
@mseri
[new release] cohttp-lwt-unix-nossl, cohttp-lwt-jsoo, cohttp, cohttp-…
8647f38 
…lwt, cohttp-lwt-unix, cohttp-lwt-unix-ssl, cohttp-top, cohttp-async and cohttp-mirage (3.0.0)

CHANGES:

- cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711)
- cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707)
- cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715)
- cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706)
- cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713)
- port to conduit 3.0.0: minor breaking changes on the server API, an explicit distinction
  between cohttp-lwt-unix (using tls), cohttp-lwt-notls and cohttp-lwt-ssl (using lwt_ssl),
  and includes ssl in cohttp-async (@dinosaure mirage/ocaml-cohttp#692)
- cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717)
- refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692)
- update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720)
- cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721)
- lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704)
- fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722)
- add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723)
- cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696)
- improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725)
- add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)
@mseri mseri mentioned this pull request Mar 24, 2021
Merged
mseri added a commit to mseri/opam-repository that referenced this pull request Mar 24, 2021
@mseri
[new release] cohttp-lwt-jsoo, cohttp, cohttp-lwt, cohttp-lwt-unix, c…
3712b50 
…ohttp-top, cohttp-async and cohttp-mirage (4.0.0)

CHANGES:

- cohttp.response: fix malformed status header for custom status codes (@mseri @aalekseyev mirage/ocaml-cohttp#752)
- Remove dependency to base (@samoht mirage/ocaml-cohttp#745)
- fix opam files and dependencies
- add GitHub Actions workflow (@smorimoto mirage/ocaml-cohttp#739)
- lwt_jsoo: Forward exceptions to caller when response is null (@mefyl mirage/ocaml-cohttp#738)
- Remove wrapped false (@rgrinberg mirage/ocaml-cohttp#734)
- Use implicit executable dependency for generate.exe (@TheLortex mirage/ocaml-cohttp#735)
- cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711)
- cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707)
- cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715)
- cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706)
- cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713)
- cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717)
- refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692)
- update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720)
- cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721)
- lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704)
- fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722)
- add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723)
- cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696)
- improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725)
- add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)
- [reverted] breaking changes to client and server API to use conduit
  3.0.0 (@dinosaure mirage/ocaml-cohttp#692). However, as the design discussion did not reach
  consensus, these changes were reverted to preserve better compatibility with
  existing cohttp users. (mirage/ocaml-cohttp#741,  @samoht)
mseri added a commit to mseri/opam-repository that referenced this pull request Mar 25, 2021
@mseri
[new release] cohttp-lwt-jsoo, cohttp, cohttp-lwt, cohttp-lwt-unix, c…
2f7b4b2 
…ohttp-top, cohttp-async and cohttp-mirage (4.0.0)

CHANGES:

- cohttp.response: fix malformed status header for custom status codes (@mseri @aalekseyev mirage/ocaml-cohttp#752)
- Remove dependency to base (@samoht mirage/ocaml-cohttp#745)
- fix opam files and dependencies
- add GitHub Actions workflow (@smorimoto mirage/ocaml-cohttp#739)
- lwt_jsoo: Forward exceptions to caller when response is null (@mefyl mirage/ocaml-cohttp#738)
- Remove wrapped false (@rgrinberg mirage/ocaml-cohttp#734)
- Use implicit executable dependency for generate.exe (@TheLortex mirage/ocaml-cohttp#735)
- cohttp: update HTTP codes (@emillon mirage/ocaml-cohttp#711)
- cohttp: add Uti.t to uri scheme (@brendanlong mirage/ocaml-cohttp#707)
- cohttp: fix chunked encoding of empty body (@mefyl mirage/ocaml-cohttp#715)
- cohttp-async: fix body not being uploaded with unchunked Async.Pipe (@mefyl mirage/ocaml-cohttp#706)
- cohttp-{async, lwt}: fix suprising behaviours of Body.is_empty (@anuragsoni mirage/ocaml-cohttp#714 mirage/ocaml-cohttp#712 mirage/ocaml-cohttp#713)
- cohttp-lwt-jsoo: rename Cohttp_lwt_xhr to Cohttp_lwt_jsoo for consistency (@mseri mirage/ocaml-cohttp#717)
- refactoring of tests (@mseri mirage/ocaml-cohttp#709, @dinosaure mirage/ocaml-cohttp#692)
- update documentation (@dinosaure mirage/ocaml-cohttp#716, @mseri mirage/ocaml-cohttp#720)
- cohttp: fix transfer-encoding ordering in headers (@mseri mirage/ocaml-cohttp#721)
- lower-level support for long-running cohttp-async connections (@brendanlong mirage/ocaml-cohttp#704)
- fix deadlock in logging (@dinosaure mirage/ocaml-cohttp#722)
- add of_form and to_form functions to body (@seliopou mirage/ocaml-cohttp#440, @mseri mirage/ocaml-cohttp#723)
- cohttp-lwt: partly inline read_response, fix body stream leak (@madroach @dinosaure mirage/ocaml-cohttp#696)
- improve media type parsing (@seliopou mirage/ocaml-cohttp#542, @dinosaure mirage/ocaml-cohttp#725)
- add comparison functions for Request.t and Response.t via ppx_compare (@msaffer-js @dinosaure mirage/ocaml-cohttp#686)
- [reverted] breaking changes to client and server API to use conduit
  3.0.0 (@dinosaure mirage/ocaml-cohttp#692). However, as the design discussion did not reach
  consensus, these changes were reverted to preserve better compatibility with
  existing cohttp users. (mirage/ocaml-cohttp#741,  @samoht)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@anuragsoni @mseri