-
Notifications
You must be signed in to change notification settings - Fork 173
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
Cohttp_lwt_unix.Server: Do not leak fd serving potentially never ending bodies #982
Conversation
>>= function | ||
| Ok () -> Lwt.return_unit | ||
| Ok () -> | ||
if keep_alive then handle_client oc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I know, it is rather undocumented unfortunately, it is still a work in progress)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Please add a changelog entry |
c6b8350
to
83fbc8c
Compare
If I am not mistaking anything, this seems fine to me. I also spent some time testing it locally and it seems all right |
…_event_ready Backport #982 to cohttp.5
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha2) CHANGES: - cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995) - http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986) - http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986) - do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985) - cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976) - cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982) - cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async and cohttp-async (6.0.0~alpha2) CHANGES: - cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995) - http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986) - http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986) - do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985) - cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976) - cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982) - cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async and cohttp-async (6.0.0~alpha2) CHANGES: - cohttp-lwt: Do not leak exceptions to `Lwt.async_exception_hook`. (mefyl mirage/ocaml-cohttp#992, mirage/ocaml-cohttp#995) - http.header, cohttp, cohttp-eio: remove "first" and "move_to_first" and the special treatment of the "host" header (mseri mirage/ocaml-cohttp#988, mirage/ocaml-cohttp#986) - http.header: introduce "iter_ord" to guarantee iteration following the order of the entries in the headers (mseri mirage/ocaml-cohttp#986) - do not omit mandatory null Content-Length headers (mefyl mirage/ocaml-cohttp#985) - cohttp-async, cohttp-curl-async: compatibility with core/async v0.16.0 (mseri, dkalinichenko-js mirage/ocaml-cohttp#976) - cohttp-lwt server: call conn_closed before drainig the body of response on error (pirbo mirage/ocaml-cohttp#982) - cohttp-eio: Relax socket interface requirement on `Server.connection_handler`. (mefyl mirage/ocaml-cohttp#983)
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:
dune exec cohttp-lwt-unix/examples/server_sent_event_lwt.exe
in one terminal and in an other I can seebut
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.