cohttp-eio: Document Server.run

[patricoferris]

Hello!

cohttp-eio is yet unreleased but there is an alpha so hopefully that means you are happy to accept feedback :))

I'm using the server in a real-world application and I think it would be good to document Server.run and all of the parameters, in particular what the default parameters are for each of the optional arguments.

I also think it would be good to make the server use only one domain to accept connections by default, leaving it up to the user to add more if they please. I had assumed this and then happened to be using Irmin in the handlers (it was a REST server on top of an irmin store) and then was surprised by some weird bugs but the tl;dr is that Irmin is not domain-safe and I was reading/writing to it from multiple domains in parallel without realising it. I feel like potentially others might do the same.

cc: @bikallem

[patricoferris]

I suppose there is an argument to be made I should have been a bit more careful considering it takes the Domain_mgr.t... it's one of the reasons I'm not a massive fan of passing in objects like:

< domain_mgr : Eio.Domain_manager.t
    ; net : Eio.Net.t
    ; clock : Eio.Time.clock
    ; .. >

Because I get lazy and just have to give it all of the env, but perhaps that's a bad habit that I need to break.

[bikallem]

Thanks for the feedback. Indeed, the API needs to be documented, possibly sprinkled with examples if the text is not clear.

I also think it would be good to make the server use only one domain to accept connections by default, leaving it up to the user to add more if they please.

Not too sure about this since in the new OCaml 5.0 world, multicore is the new default. But yes, the default value need to be documented.

[patricoferris]

Ah actually I misread the code and the error was a domain problem I created myself apologies, the cohttp server is single threaded by default my bad (unless COHTTP_DOMAINS is set, which should maybe also be documented). I've removed that from the title of the issue.

Regarding the use of env:

ocaml-cohttp/cohttp-eio/src/cohttp_eio.mli

Line 99 in ec82f50

'a env ->

After a cursory glance, I don't think the connection handler needs everything in env just the clock?

[talex5]

I'm using the server in a real-world application and I think it would be good to document Server.run and all of the parameters

I think we should move Server.run's functionality to Eio itself.

Though I'm not a big fan of creating the socket and running the accept loop in a single function. You almost always want to split those (e.g. to allow receiving a listening socket from systemd, or to send a notification that the socket is ready after you start listening but before entering the loop). Also, both parts may have lots of options (e.g. to set various socket options on the listening socket and to control how many concurrent users to allow in the loop), so splitting it in two might make the code easier to read.

After a cursory glance, I don't think the connection handler needs everything in env just the clock?

Yes, looks like this should just be taking a clock.

[bikallem]

I think we should move Server.run's functionality to Eio itself.

I am open to this. Do you want the the function moved to eio as is without the functionality of getting domain count from environment? i.e. with the splitting as you suggested.

[bikallem]

Just to clarify on the above point. I think even after moving some of the functionality of Server.run to eio, there will still be a thin version of Server.run albeit using most of the functionality from eio, for eg. implementing COHTTP_DOMAINS env setting which I found very useful to set domains during benchmarking tasks.

[patricoferris]

implementing COHTTP_DOMAINS env setting which I found very useful to set domains during benchmarking tasks

I find it a little strange for a library to be getting things from the environment, it seems like something you should add to your binaries (dune executables) that you then pass to the library function (what is currently happening in server.ml).

[talex5]

We'll probably want to change a few other things:

• There should probably be separate functions for running with 1 domain vs multiple domains. As @patricoferris said, using multiple domains will lead to bugs if you're not expecting it and haven't confirmed that the handler is thread-safe.

• reuse_port is a dangerous option that can lead to having old and new versions of your server running at once without realising, which is very confusing.

• We also need to sort out sharing of the socket between domains, as this currently crashes the luv backend (Luv: deadlock sharing resources between domains? ocaml-multicore/eio#386). Probably the solution here is to replace libuv with something saner.

• It should allow you to limit the number of concurrent connections.

[bikallem]

It should allow you to limit the number of concurrent connections.

What should happen if we reach/breach the specified number of connections? sleep and try accepting again?

[talex5]

I was imagining something like:

let pool = Semaphore.make max_connections in
while true do
  Semaphore.acquire pool;
  Net.accept_fork ... (fun flow addr ->
    ...
    Semaphore.release pool
  )
done