search

c-cube / tiny_httpd

Minimal HTTP server using good old threads + blocking IO, with a small request router.
https://c-cube.github.io/tiny_httpd
75 stars 11 forks source link

Stream do not close the file descriptor #52

Closed craff closed 1 year ago

craff commented 1 year ago

This piece of code in Tiny_httpdserver (function output does not close the stream str) Could be a problem if a client keep a connection for a long time.

    begin match body with
      | `String "" | `Void -> ()
      | `String s -> output_string oc s;
      | `Stream str -> Byte_stream.output_chunked oc str;
    end;
    flush oc
craff commented 1 year ago

The command wrk -t 5 -c100 -d15 url on a small file gives a lot of 500 codes probably because of this.

c-cube commented 1 year ago

I think it's good to close the connection if an option is set (--no-connection-reuse or whatever). Otherwise, a client might send another query. We might be able to set a timeout on how long it takes to send the new query, I'm not sure.

We have problems with slow lorris and other things, I'm afraid. In general it's going to be hard to be iron solid. A reverse proxy should be the way to do that cleanly if you're exposed to the public internet, and it can also handle TLS.

craff commented 1 year ago

I don't understand ? What is slow lorris ? It does not seem costly to close the stream ? I don't speak about the connection socket, but the stream of the file being send over the socket. I just submitted PR #55, which is rather trivial.

craff commented 1 year ago

Without this PR #55, a long running connection downloading many files will fail with EMFILE (no more file descriptor). It is quite strange that this bug was not detected actually.

c-cube commented 1 year ago

https://en.wikipedia.org/wiki/Slowloris_(computer_security)

but here you're talking about the stream in a response, which is not closed after being written back to the client, is that it? I guess it can be a leak indeed. I'm not sure why it did not pop up before; maybe we only did load testing with a server that didn't use streams with underlying FDs.

c-cube commented 1 year ago

I think this is fixed by #55