Cohttp_lwt_unix.Server: Do not leak fd serving potentially never ending bodies

[pirbo]

Please tell me if I should be doing what I do in a complete different way (and c70ad81 makes me think I do because Cohttp.Connection are useful in my case)

I want to use Cohttp to serve Server Sent Events (aka stream of things living as long as there is a client to receive them) and the way I implement them follows the sketch of 3226fdd but I realize that without this patch "Cohttp" never close the connections:

• I dune exec cohttp-lwt-unix/examples/server_sent_event_lwt.exe in one terminal and in an other I can see

$ lsof -p PID | wc -l
15
$ curl http://localhost:8800
Not found: //localhost:8800/
$ lsof -p PID | wc -l
15
$ curl http://localhost:8800/lol
Not found: //localhost:8800/lol
$ lsof -p PID | wc -l
15

but

$ curl http://localhost:8800/tic
Tic
Tic
^C
$ lsof -p PID | wc -l
16
$ curl http://localhost:8800/tic
Tic
^C
$ lsof -p PID | wc -l
17

And obviously, if you wait to drain the infinite body of the reply before calling conn_closed that would terminates it, it would take a while...

So, here I'm (maybe naively) swapping the 2.

[mseri]

If handle_client fails to read here we will never drain the body. This was related to an issue that led to a DOS: #977

[mseri]

I am not against this change, but we need to make it in such a way as to not re-open #978

The rule of thumb was that there shoudl be no infinite body, and if tje server has it, there should be a server-side way to prevent it. Maybe in this case it could be more suitable to use https://github.com/mirage/ocaml-cohttp/blob/master/cohttp-server-lwt-unix/src/cohttp_server_lwt_unix.mli which has a finer control on the body?

[mseri]

(I know, it is rather undocumented unfortunately, it is still a work in progress)

[pirbo]

cohttp-server-lwt-unix looks promising and clearly it may be the way, going forward.

That being said, I think this patch does not reopen #978: If IO.catch [...]returns Ok (), it means that Response.write [...] succeed which means that the Body.write_body [...] have all been successfully completed so this body of that response fully consumed and there is no more things to drain here, isn't it ?

Edit: I forgot to thank you for your reactivity, Thank you!

[mseri]

You are right, I went too fast there. I think it is fine. I wonder if we can add a test case, some kind of update of https://github.com/mirage/ocaml-cohttp/pull/977/files#diff-efd414a623c8f96d3b50c273441dd9a953d0d721a3369f4588ba01a3d1a0710d perhaps using the example you linked.

[mseri]

Please add a changelog entry

[mseri]

If I am not mistaking anything, this seems fine to me. I also spent some time testing it locally and it seems all right