Conduit 3.0.0

[dinosaure]

A draft about a move to conduit.3.0.0. For some details, see mirage/ocaml-conduit#311.

Currently, the new version of conduit comes with 2 major updates (just about implementation):

• the global loop is not defined by conduit anymore. Implementation must write it - and take care about how to finish the loop, how to close connections, etc.
• the ctx is, conceptually, replaced by resolvers (available into the conduit package) and must be filled by the user to be able to resolve something. The previous version of conduit has a side-effect on the ctx and can be filled in many ways - specially by application of functor in the context of mirage. Now conduit gives only an empty resolvers and it must be filled by the user at the end - as I do in this PR.

Of course, 2 others updates appear:

• the ability to destruct a flow to a concrete type from a given protocol implementation (which is define by the user only)
• the ability to compose with conduit-tls (which is outside the scope of conduit-lwt or conduit-lwt-unix, it depends only on conduit) a protocol with TLS (and ocaml-tls). Examples are done into binaries provided by cohttp-lwt-unix`

It still a draft and I'm currently on:

• mirage support
• async support
• js_of_ocaml support

[dinosaure]

mirage support is done. However, an orthogonal issue exists where conduit decides to update the mirage-flow interface to let the user to allocate the input buffer. conduit-mirage provides a mirage-flow interface as expected by mirage-channel - however it seems that mirage-channel is used only by cohttp.

We should delete the use of mirage-channel (or update it according the new given interface of the flow provided by conduit).

[dinosaure]

Trust on ocaml-ci instead Travis CI (much more faster).

[dinosaure]

Ok, ocaml-ci is happy! It failed before due to an overlap of cohttp servers on the same port 9193 but it fixed now with the last commit. I think, the diff is small as I can. Please someone to review this PR (@mseri, @avsm) I'm not really confident with cohttp-async when I don't use it.

It still is a draft when we need to add a submodule to be able to compile correctly cohttp. Once conduit is released, I will clean this PR.

[mseri]

Thanks for the big effort!

I am going to review it next week, as soon as I can. Is there a place where I can find documentation for the new conduit already accessible?

Note for myself: before merge

• update the examples in the README and any other snippet around if needed (we should really test these...)
• make a new release of cohttp before 3.0, if not already done

[mseri]

I will likely merge a few PRs before the next release, including #698 or a variation of it, sorry for the potential merge conflicts

[dinosaure]

Yes, of course, I can make a pass then to avoid any conflicts.

[dinosaure]

The PR was rebased with the new trunk, however @avsm wants to look something about conduit/cohttp.

[mseri]

Thanks. Can you point me to the documentation and design of conduit 3, so that I can study a bit before embarking in the review?

[dinosaure]

Currently, the README.md of conduit is not well updated to the new interface but a global view is available here if you want - note that I will make something more accurate with the new interface of conduit.

[mseri]

I have done a first superficial review. Sorry it took so long. A blog post on the server side of conduit would be very useful!

[dinosaure]

Thanks for your report.

What was interrupt for? Is there a reason for keep it there unused?

Similarly as in the other case, the new conduit is dropping max_connections. backlog and buffer_age_limit. Is this something to worry about? Is there a reason for keep it here if not?

As before, we are ditching timeout and backlog. Is this something to worry about? Should we just remove them?

So all of these questions point to a regression about what it should be done by conduit and cohttp.

Come back to the rewrite of conduit, I discovered an hard effort to provide a server_with_handler close to what Async (see create) provides. . Conduit tried to provide the same logic on both sides (conduit-lwt and conduit-async).

On other side, it seems clear that we can not /mimic/ (or reproduce) the same behavior than async on conduit-lwt with such complexity (many arguments and all of them slightly change our loop). A main difference between lwt and async is highlighted by this required wait on conduit's tests.

From the perspective of conduit, I would not provide a common and complex way to start a server loop (even if I added it!) to let the user to really do what he wants (and take the responsibility on it).

A blog post on the server side of conduit would be very useful!

So more concretely, conduit provides what it should be expected by the user to create its own server loop (bind & listen, accept and close). Service uses the abstraction as Protocol but all types are well-known (this is the main difference from the old version of conduit) and can be concretely constructed/destructed.

Finally, I would like to not provide more than this to still be homogeneous over lwt, async and mirage - and some tricks are required to ensure this homogenization between these back-ends.

Recently, some others issues were discovered in mirage/irmin#1062 and mirage/irmin#1066 where it's really hard to figure out at which layer these bugs appear. This recent misadventure gives me a more strong opinion about this question. HTTP/AF gives an other opinion where the server loop can be more complex than what we can naively provide - and it depends on which protocol we want to implement.

So about cohttp, I think the best is to shift the complexity of the server loop into cohttp. More concretely:

• backport is the capacity or in other words, the number given to listen
• timeout is a sleep and if the accept take more than timeout, we shutdown the server
• max_connections appears only on the async side
  However, the current old version of conduit-lwt-unix can limit the number of clients with set_max_active (which is not used by cohttp-lwt-unix).
• on_exn (cohttp-lwt)/on_handler_error (conduit-async) could be add into conduit
• buffer_age_limit exists only on the async side
• interrupt exists only on the async side (only the client part)

So I can try to solve these issues and provide something on cohttp (on this PR) to still continue to handle these options. However, we should decide what it should be common and deleted between cohttp-async and cohttp-lwt (@avsm?) - or assert a difference between cohttp-async and cohttp-lwt (in that case, cohttp-async should directly use Async_unix.Tcp instead to use conduit-async).

[dinosaure]

So I updated my PR such as:

• ?backlog is used only when the user wants to start a TCP connection. The integer is given to listen properly as before. However, we need to clarify a point: ?backlog leads the configuration.
• ?timeout is added too into conduit-{lwt,async}. I took the opportunity to /homogenize/ the API between cohttp-async and cohttp-lwt-unix, both have the ?timeout argument
• ?interrupts/?max_connections/?buffer_age_limit are too close to Async, so I removed them (only into cohttp-async of course). As I said, we can consider this update as a regression however: if it's really required, I can propose an other PR which will implement such arguments for both back-ends

Finally, an error occurs on 32-bits architectures but it's unrelated to this PR. I will clean the PR (about history) and I think it will be ready to merge. If you have any others feedbacks @mseri, tell me 👍.

• clean the PR

[mseri]

Thanks for the update and the clarifications. I agree with your comments and look forward hearing the opinion of other users and maintainers.

I have just a small request and two questions.

• do the examples (https://github.com/mirage/ocaml-cohttp#client-tutorial) in the tutorial need fixing to work in the new cohttp word? (I cannot create the switch to check it properly at the moment --and for a while unfortunately--, otherwise I would have looked at them myself)
• do you think that the new conduit and this revamp of cohttp can solve the issue pointed out here: Lower-level support for long-running cohttp-async connections #704?
• do think that with you the new conduit and this revamp of cohttp we can also address the issue pointed out here: partly inline read_response, fix body stream leak #696?

[dinosaure]

do the examples (https://github.com/mirage/ocaml-cohttp#client-tutorial) in the tutorial need fixing to work in the new cohttp word? (I cannot create the switch to check it properly at the moment --and for a while unfortunately--, otherwise I would have looked at them myself)

Surely, yes. If I take the example as is, conduit will be not able to resolve anything (due to Conduit.empty). A solution, and I think @avsm agree with that, is to define Net.empty with, at least, the host-system DNS resolver to be able, at least, to create a TCP/IP (*:80) connection. I don't have a strong opinion with that so I can add it into cohttp-async and cohttp-lwt.

About cohttp-async and cohttp-lwt-__unix__, we can extend the resolution (Conduit.resolvers) with an SSL support (*:443) with conduit-{async,lwt}-ssl (in anyway, conduit-async and conduit-lwt-unix depend on these packages).

• add a default resolvers which include resolution of TCP connection and SSL connection

do you think that the new conduit and this revamp of cohttp can solve the issue pointed out here: #704?

From my perspective, the ability to use something else (defined by the user) than a simple socket such as, in this case, a pool of socket/connection is possible - but it's not well documented and it's not the part of conduit to propose such implementation. The initial issue should be fixed in a different path where the user should implement its own protocol, register it into conduit and re-extract/de-abstract it as well in callv.

So it will not fix the initial issue (where conduit provides something simpler/easier to maintain) but it unlocks the ability to inject an another (new) kind of connection (with another impl. of connect/recv/send/close) into cohttp independently than conduit and cohttp.

So for me, we should keep #704 and may be rethink it in the scope of conduit3.0.0.

do think that with you the new conduit and this revamp of cohttp we can also address the issue pointed out here: #696?

It seems that this specific issue is out of the scope of the purpose of cohttp + conduit.3.0.0 and the issue is more about the usage of Lwt_io.channel (already provided by conduit).

[mseri]

Thank you very much for the fast response! Let's keep #704 open and see if we can rethink it then.

I agree that having a default DNS resolver would be incredibly useful for http(s) connections. I think we should add a paragraph in the documentation referring to the appropriate conduit section for people that want to implement their own custom resolver.

[mseri]

Do we still plan to re-add a cohttp-lwt-unix-notls package?

[dinosaure]

Do we still plan to re-add a cohttp-lwt-unix-notls package?

So, according to the last commit, the change will be on Net.connect_uri - and raise invalid_arg if the user give an https URL - so, I can do that now, yes.

[hannesm]

I added trust anchor verification using the ca-certs package in dinosaure#2

[dinosaure]

mirage/ocaml-conduit#328 was updated properly to fix the server-side. The main problem is the use of Lwt_io which wants to start to read in any context - even if the underlying socket is not Lwt_unix.readable. So I did some tweak on conduit-tls and conduit-lwt (TCP modules) to properly start to read if we are the server and properly start to send if we are the client. The TLS layer gives the opportunity to re-schedule the read/write to the io_of_flow function used by cohttp-lwt then.

• Behaviors described by @hannesm work (as I wrote before).
• A TLS server or a simple HTTP server work too from my tests (on my computer and on my server).

However, I would like a double check when the problem is about scheduling between the read and the write used by Lwt_io. Then, this issue highlights that we should not use Lwt_io finally - I mean, if we want to properly handle an HTTP/1.1, we should have a full control of when we want to read and when we want to write.

[mseri]

The extra test that I am making is the following:

git clone https://github.com/mseri/doi2bib/
cd doi2bib
opam install . --deps-only
dune runtest

Unfortunately most test cases (but not all!) are still failing with the latest version of this PR and conduit 3.0.0:

$ dune exec bin/doi2bib.exe -- 1902.00436        
Fatal error: exception (Failure "Client connection was closed")

The only call that is not failing is

$ dune exec bin/doi2bib.exe -- arXiv:math/0606217
@misc{Marklof2006Distribution,
      title={Distribution modulo one and Ratner's theorem}, 
      author={Jens Marklof},
      year={2006},
      eprint={math/0606217},
      archivePrefix={arXiv},
      primaryClass={math.NT}
}

which is the only one not calling the DOI registry with a custom header.

With the old cohttp these all worked: I just retried both CONDUIT_TLS=native dune runtest and CONDUIT_TLS=openssl dune runtest with the current released cohttp/conduit and both work as expected.

[dinosaure]

@mseri according to mirage/ocaml-conduit#329 and the last patch, your tool works perfectly (with ocaml-tls). However, it seems more trickier about OpenSSL support currently, I think we should provide cohttp-lwt-unix-ssl and cohttp-lwt-unix-notls (as you suggested) to fix a global and systemic error: the use of Lwt_io.

The error did not appear before because we were able to make Lwt_io values from an Lwt_ssl.socket and mostly because the old version of conduit was not really abstracted (for this point, due to the choice at the compile time of which implementation of TLS we wanted, the old version of conduit was able to directly provide Lwt_io values from the well-known Lwt_ssl.socket - however, with the new version of conduit which is fully-abstract, we must rewrap Lwt_io with Conduit.send and Conduit.recv).

About the idea to make cohttp-lwt-unix-ssl, it's just to have the ability (like before) to get Lwt_io values in an other way than Conduit_lwt.io_of_flow (which does not work in anyway for a Conduit_lwt_ssl.Protocol.flow) - and, if we have a Conduit_lwt_ssl flow, we are able to destruct it and make Lwt_io values from Lwt_ssl.{out,in}_channel_of_descr.

About the error with ocaml-tls, it's mostly due to that doi.org requires at the handshake the peer name (see 7d56281). Otherwise, it fails.

With these patches and the idea, I can use doi2bib with ocaml-tls and OpenSSL as before 🎉. I will make these packages tomorrow.

[mseri]

Awesome. Thank you very much for looking into it so fast.
Indeed, now it works perfectly

I like the plan! Look forward for the final changes so that we can finally merge this

[mseri]

@dinosaure if you remove the pin-depends, this is ready for merge

[mseri]

Thanks. I will wait for the CI before merging, just to double check that all is good with the new opam-repository version

[mseri]

Before making a new release there are two small breaking changes from previous PRs that I want to bring back but it should not take long

[mseri]

Thanks again for such big effort