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

[new release] cohttp, cohttp-top, cohttp-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo and cohttp-async (5.0.0) #20246

Merged
merged 4 commits into from
Jan 5, 2022

Conversation

mseri
Copy link
Member

@mseri mseri commented Dec 15, 2021

An OCaml library for HTTP clients and servers

CHANGES:
  • Cohttp.Header: new implementation (lyrm Associative list implementation for headers mirage/ocaml-cohttp#747)

    • New implementation of Header modules using an associative list instead of a map, with one major semantic change (function get, see below), and some new functions (clean_dup, get_multi_concat)
    • More Alcotest tests as well as fuzzing tests for this particular module.

    Purpose

    The new header implementation uses an associative list instead of a map to represent headers and is focused on predictability and intuitivity: except for some specific and documented functions, the headers are always kept in transmission order, which makes debugging easier and is also important for RFC7230§3.2.2 that states that multiple values of a header must be kept in order.

    Also, to get an intuitive function behaviour, no extra work to enforce RFCs is done by the basic functions. For example, RFC7230§3.2.2 requires that a sender does not send multiple values for a non list-value header. This particular rule could require the Header.add function to remove previous values of non-list-value headers, which means some changes of the headers would be out of control of the user. With the current implementation, an user has to actively call dedicated functions to enforce such RFCs (here Header.clean_dup).

    Semantic changes

    Two functions have a semantic change : get and update.

    get

    get was previously doing more than just returns the value associated to a key; it was also checking if the searched header could have multiple values: if not, the last value associated to the header was returned; otherwise, all the associated values were concatenated and returned. This semantics does not match the global idea behind the new header implementation, and would also be very inefficient.

    • The new get function only returns the last value associated to the searched header.
    • get_multi_concat function has been added to get a result similar to the previous get function.

    update

    update is a pretty new function (Improve Header.update mirage/ocaml-cohttp#703) and changes are minor and related to get semantic changes.

    • update h k f is now modifying only the last occurrences of the header k instead of all its occurrences.
    • a new function update_all function has been added and work on all the occurrences of the updated header.

    New functions :

    • clean_dup enables the user to clean headers that follows the {{:https://tools.ietf.org/html/rfc7230#section-3.2.2} RFC7230§3.2.2} (no duplicate, except set-cookie)
    • get_multi_concat has been added to get a result similar to the previous get function.
  • Cohttp.Header: performance improvement (mseri, anuragsoni cohttp.headers: use faster comparison mirage/ocaml-cohttp#778)
    Breaking the headers are no-longer lowercased when parsed, the headers key comparison is case insensitive instead.

  • cohttp-lwt-unix: Adopt ocaml-conduit 5.0.0 (smorimoto Adopt ocaml-conduit 5.0.0 mirage/ocaml-cohttp#787)
    Breaking Conduit_lwt_unix.connect's ctx param type chaged from ctx to ctx Lazy.t

  • cohttp-mirage: fix deprecated fmt usage (tmcgilchrist Fix deprecated Fmt usages. mirage/ocaml-cohttp#783)

  • lwt_jsoo: Use logs for the warnings and document it (mseri lwt_jsoo: use log instead of always printing the warnings on the console mirage/ocaml-cohttp#776)

  • lwt: Use logs to warn users about leaked bodies and document it (mseri lwt.client: log drain body warning instead of printing it to stderr mirage/ocaml-cohttp#771)

  • lwt, lwt_unix: Improve use of logs and the documentation, fix bug in the Debug.enable_debug function (mseri Revisit logs in cohttp-lwt mirage/ocaml-cohttp#772)

  • lwt_jsoo: Fix exception on connection errors in chrome (mefyl Fix exception on connection errors in chrome. mirage/ocaml-cohttp#761)

  • lwt_jsoo: Fix Lwt.wakeup_exn Invalid_arg exception when a js
    stack overflow happens in the XHR completion handler (mefyl Fix Invalid_arg exception on stack overflow when waking up a request. mirage/ocaml-cohttp#762).

  • lwt_jsoo: Add test suite (mefyl Add cohttp-lwt-jsoo test suite. mirage/ocaml-cohttp#764).

@mseri
Copy link
Member Author

mseri commented Dec 15, 2021

This should be merged only once it is properly tested in the CI, and right now the CI is overloaded

mseri and others added 3 commits December 17, 2021 09:24
…wt-unix, cohttp-lwt-jsoo and cohttp-async (5.0.0)

CHANGES:

- Cohttp.Header: new implementation (lyrm mirage/ocaml-cohttp#747)

  + New implementation of Header modules using an associative list instead of a map, with one major semantic change (function ```get```, see below), and some new functions (```clean_dup```, ```get_multi_concat```)
  + More Alcotest tests as well as fuzzing tests for this particular module.

  ### Purpose

  The new header implementation uses an associative list instead of a map to represent headers and is focused on predictability and intuitivity: except for some specific and documented functions, the headers are always kept in transmission order, which makes debugging easier and is also important for [RFC7230§3.2.2](https://tools.ietf.org/html/rfc7230#section-3.2.2) that states that multiple values of a header must be kept in order.

  Also, to get an intuitive function behaviour, no extra work to enforce RFCs is done by the basic functions. For example, RFC7230§3.2.2 requires that a sender does not send multiple values for a non list-value header. This particular rule could require the ```Header.add``` function to remove previous values of non-list-value headers, which means some changes of the headers would be out of control of the user. With the current implementation, an user has to actively call dedicated functions to enforce such RFCs (here ```Header.clean_dup```).

  ### Semantic changes
  Two functions have a semantic change : ```get``` and ```update```.

  #### get
  ```get``` was previously doing more than just returns the value associated to a key; it was also checking if the searched header could have multiple values: if not, the last value associated to the header was returned; otherwise, all the associated values were concatenated and returned. This semantics does not match the global idea behind the new header implementation, and would also be very inefficient.

  + The new ```get``` function only returns the last value associated to the searched header.
  + ```get_multi_concat``` function has been added to get a result similar to the previous ```get``` function.

  #### update
  ```update``` is a pretty new function (mirage/ocaml-cohttp#703) and changes are minor and related to ```get``` semantic changes.

  + ```update h k f``` is now modifying only the last occurrences of the header ```k``` instead of all its occurrences.
  + a new function ```update_all``` function has been added and work on all the occurrences of the updated header.

  ### New functions :

  + ```clean_dup```  enables the user to clean headers that follows the {{:https://tools.ietf.org/html/rfc7230#section-3.2.2} RFC7230§3.2.2} (no duplicate, except ```set-cookie```)
  + ```get_multi_concat``` has been added to get a result similar to the previous ```get``` function.

- Cohttp.Header: performance improvement (mseri, anuragsoni mirage/ocaml-cohttp#778)
  **Breaking** the headers are no-longer lowercased when parsed, the headers key comparison is case insensitive instead.

- cohttp-lwt-unix: Adopt ocaml-conduit 5.0.0 (smorimoto mirage/ocaml-cohttp#787)
  **Breaking** `Conduit_lwt_unix.connect`'s `ctx` param type chaged from `ctx` to  `ctx Lazy.t`

- cohttp-mirage: fix deprecated fmt usage (tmcgilchrist mirage/ocaml-cohttp#783)
- lwt_jsoo: Use logs for the warnings and document it (mseri mirage/ocaml-cohttp#776)
- lwt: Use logs to warn users about leaked bodies and document it (mseri mirage/ocaml-cohttp#771)
- lwt, lwt_unix: Improve use of logs and the documentation, fix bug in the Debug.enable_debug function (mseri mirage/ocaml-cohttp#772)
- lwt_jsoo: Fix exception on connection errors in chrome (mefyl mirage/ocaml-cohttp#761)
- lwt_jsoo: Fix `Lwt.wakeup_exn` `Invalid_arg` exception when a js
  stack overflow happens in the XHR completion handler (mefyl mirage/ocaml-cohttp#762).
- lwt_jsoo: Add test suite (mefyl mirage/ocaml-cohttp#764).
@mseri
Copy link
Member Author

mseri commented Dec 17, 2021

ez_api conflicts were incorrect, fixed in #20282
h1_parser was also fixed in #20276

The rest is fine, this can be merged

@kit-ty-kate
Copy link
Member

The constraints on zeit and h1_parser do not seem to be right. The same errors happen and they are still marked as compatible with this release

@mseri
Copy link
Member Author

mseri commented Dec 23, 2021

I think that was run with the incorrect fix, let me restart the CI

@kit-ty-kate
Copy link
Member

It’s not the CI, i already restarted it twice

@mseri
Copy link
Member Author

mseri commented Dec 23, 2021

Oh, then it is quite suprising. I don't think there were breaking changes in the Headers for quite a while

@mseri
Copy link
Member Author

mseri commented Dec 23, 2021

The CI is installing cohttp 5.0.0 even if there is a conflict for cohttp >= 5

@kit-ty-kate
Copy link
Member

Sadly there isn’t. See my comment on #20276 for example

@mseri
Copy link
Member Author

mseri commented Dec 23, 2021

I have just seen your comments... I hope those constraints will be recognized in the future.
What do you think? Should I disable the tests if cohttp:version >= 5.0.0, or we just leave them there as is until they will be added.

@kit-ty-kate
Copy link
Member

I think it’s easier to disable them entirely (by simply removing the with-test part) for now and see later to unlock them for cohttp 5

@mseri
Copy link
Member Author

mseri commented Dec 24, 2021

I think this can be merged. Zeit can be fixed separately (issue opened here: #20319)

@mseri mseri merged commit 57c9f57 into ocaml:master Jan 5, 2022
@mseri mseri deleted the release-cohttp-v5.0.0 branch January 5, 2022 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants