max_keep_alive and non blocking reading of request #23

craff · 2021-12-08T22:08:24Z

I experienced bad behavior of my server due to client not closing sockets (and a proxy makes it worth). Just increasing
max_connection is not enough, closing the navigator still does no close the socket. I needed two fixes to handle that:

I added a max_keep_alive to automatically close socket if there is no new request for some time (defaut is -1.0)
not to break existing code.
the reading of request is now using Unix.read and a non blocking socket to make sure we are not blocking on Stdlib.input
which was a behavior I observed very frequently.

… not to loose connection slot. Default is max_keep_alive = -1.0 which keeps the original behaviour.

c-cube

That looks pretty good for a first draft, I think we can cleanup a bit (particularly the debug code?) but this should be merged I hope.

src/Tiny_httpd.ml

src/Tiny_httpd.mli

src/Tiny_httpd.ml

c-cube · 2021-12-09T01:38:55Z

src/Tiny_httpd.ml

@@ -83,6 +83,27 @@ module Byte_stream = struct
  let of_chan = of_chan_ ~close:close_in
  let of_chan_close_noerr = of_chan_ ~close:close_in_noerr

+  exception Timeout
+
+  let of_descr_ ?(timeout=(-1.0)) ~close ic : t =


that looks good. We might be able to remove of_chan entirely, I think, if we rely on Unix-level IOs.

There is still one place when of_chan is used ... I didnot want to change to many things.

c-cube · 2021-12-09T01:39:21Z

src/Tiny_httpd.ml

+  let of_descr_ ?(timeout=(-1.0)) ~close ic : t =
+    let i = ref 0 in
+    let len = ref 0 in
+    let buf = Bytes.make 4096 ' ' in


bigger buffers are probably nice, like 16 kb at least :). Not your fault, I used this value before.

Maybe we can make a new parameter to create. A server handling many small request will prefer smaller buffer than
a server handling big, but less many request. So it is nive to let the user choose ?

I also thought using your buffered input above ocaml's channel was stacking buffer ... So my PR also addresses that.

funny, I thought the same, and wrote this today. turns out, it's not faster ☹️

in_channel use a large buffer (64Kb from memory) and is written in C, I am not very surprised... It is not slower at least ;-)

src/Tiny_httpd.ml

src/Tiny_httpd.mli

Co-authored-by: Simon Cruanes <simon.cruanes.2007@m4x.org>

c-cube

still a small change, but looking good!

src/Tiny_httpd.ml

craff · 2021-12-09T03:49:28Z

I removed the mutex.

craff · 2021-12-09T03:55:03Z

I made available_connections available to the enduser by my latest commit.

craff · 2021-12-09T04:11:05Z

I did further tests:

-1.0 is a good default if behind an apache proxy that keeps a pool of connection ... But max_connections should be compatible
with apache configuration. apache really does not like the remote server to shutdown connection with the default parameter.
With a server attacked directly by web client (especially firefox) a positive default is recommended

May be a word should be added in the documentation of max_connections and max_keep_alive ?

c-cube · 2021-12-10T21:51:35Z

src/Tiny_httpd.ml

+        i := 0;
+        let (to_read,_,_) = Unix.select [ic] [] [] timeout in
+        if to_read = [] then raise Timeout;
+        try len := Unix.read ic buf 0 (Bytes.length buf)


btw, should this be in a loop?

Our file descriptor are sockets with an internal default buffer size of:
On 64-bit Linux 4.14.18: 212992 bytes
On 32-bit Linux 4.4.92: 163840 bytes

So if read returns less than those value, it means that the writer has currently written less that this number of bytes and the best is to return so that the scanning of the request can proceed, while the writer may continue to write, if the request was not send completely yet.

For a real file, read will "almost always" return the number of bytes required (except at the end of file).

The best would be to use the same buffer size for our buffer and the socket buffer using (or ensure at least a greater size)

val getsockopt_int : file_descr -> socket_int_option -> int
(** Same as {!getsockopt} for an integer-valued socket option. *)

val setsockopt_int : file_descr -> socket_int_option -> int -> unit
(** Same as {!setsockopt} for an integer-valued socket option. *)

sorry, I mean the try. Should it be in a loop so that in case of exception (EAGAIN) we try to read again?

…never knowns

craff · 2021-12-10T22:53:01Z

I reverted to acquire before accept, but adjusting available connection with a +1.

c-cube · 2021-12-11T14:35:19Z

I tried to merge, but then I realized there's a deep issue here. There is a single FD per client, which means that all writing to the socket should also be non blocking (trying it yields Sys_blocked_io quite quicky).

The current state of things is now in a branch wip-nonblock , to which I can give you access if you want. It needs more work to figure out how to adapt everything to nonblocking, I suppose.

We also need much, much stronger tests for now.

craff · 2021-12-11T20:28:56Z

I thought about that. It is more work to do non blocking write. But the previous version is worth, more chance to have client/server blocked if we do both blocking read and write than if we do just non blocking read. I actually clearly observed such a blocked situation with previous code. My problem turned out not to be a timeout issue, as I now use -1.0 for the select and do not loose a single connection. Cheers, Christophe Le 11 décembre 2021 04:35:29 GMT-10:00, Simon Cruanes ***@***.***> a écrit :

…

I tried to merge, but then I realized there's a deep issue here. There is a single FD per client, which means that all _writing_ to the socket should also be non blocking (trying it yields `Sys_blocked_io` quite quicky). The current state of things is now in a branch wip-nonblock , to which I can give you access if you want. It needs more work to figure out how to adapt everything to nonblocking, I suppose. We also need much, much stronger tests for now. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #23 (comment)

craff · 2021-12-11T20:45:11Z

Apriori, just after a select there are data in the socket buffer and there will be no eagain or ewouldblock, which are by the way are identical and there only for historical reasons (at&t versus IBM). Le 11 décembre 2021 04:01:25 GMT-10:00, Simon Cruanes ***@***.***> a écrit : ***@***.*** commented on this pull request.

…

> @@ -83,6 +83,27 @@ module Byte_stream = struct let of_chan = of_chan_ ~close:close_in let of_chan_close_noerr = of_chan_ ~close:close_in_noerr + exception Timeout + + let of_descr_ ?(timeout=(-1.0)) ~close ic : t = + let i = ref 0 in + let len = ref 0 in + let buf = Bytes.make 4096 ' ' in + { bs_fill_buf=(fun () -> + if !i >= !len then ( + i := 0; + let (to_read,_,_) = Unix.select [ic] [] [] timeout in + if to_read = [] then raise Timeout; + try len := Unix.read ic buf 0 (Bytes.length buf) sorry, I mean the `try`. Should it be in a loop so that in case of exception (`EAGAIN`) we try to read again? -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #23 (comment)

c-cube · 2021-12-11T21:07:25Z

@craff question for your problem, too: would it be legit for the server to have a mode where, after the first request/response on a socket, the server closes the connection explicitly? unless keep-alive is specified? I forget if the http specs allow that.

craff · 2021-12-12T06:33:10Z

On 21-12-11 06:01:25, Simon Cruanes wrote: @c-cube commented on this pull request. ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ In src/Tiny_httpd.ml: > @@ -83,6 +83,27 @@ module Byte_stream = struct let of_chan = of_chan_ ~close:close_in let of_chan_close_noerr = of_chan_ ~close:close_in_noerr + exception Timeout + + let of_descr_ ?(timeout=(-1.0)) ~close ic : t = + let i = ref 0 in + let len = ref 0 in + let buf = Bytes.make 4096 ' ' in + { bs_fill_buf=(fun () -> + if !i >= !len then ( + i := 0; + let (to_read,_,_) = Unix.select [ic] [] [] timeout in + if to_read = [] then raise Timeout; + try len := Unix.read ic buf 0 (Bytes.length buf)

I looked at your code in wip-nonblock. No point to make a difference between EAGAIN and EWOULDBLOCK: /usr/include/asm-generic/errno.h:#define EWOULDBLOCK EAGAIN /* Operation would block */ A case of EAGAIN/EWOULDBLOCK after select (which is very rare) may be a bad checksum or ressource temporarely locked by the kernel. The best is to always return in the select to let the kernel finish its job or the client to resend. Your FIXME is probably not necessary, the exception will rarely occur and we can spare a call to gettimeofday ? I am pushing a PR on the wip-nonblock branch containing - a merge of master - a fix of your test (kill %1 not working on my machine) - a first working non blocking buffered write (according to test) - buffer (read and write) have size of the socket buffer - needs cleanup before merge

…

sorry, I mean the try. Should it be in a loop so that in case of exception (EAGAIN) we try to read again? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. *

-- Christophe Raffalli tél: +689 87 23 11 48 web: http://raffalli.eu

craff · 2021-12-12T10:12:07Z

On 21-12-11 13:07:36, Simon Cruanes wrote: @craff question for your problem, too: would it be legit for the server to have a mode where, after the first request/response on a socket, the server closes the connection explicitly? unless keep-alive is specified? I forget if the http specs allow that.

This is HTTP 1.0 So Tinihttpd should close the connection in case of HTTP 1.0 and no keep-alive in the header (which was introduced in 1.0, before 1.1) Moreover, in HTTP 1.1 the request could include Connection: close in which case the server should close the connection. Finally if you want a mode where the server closes all connections after the answer, you can include Connection: close in the Response header. This could be an option in create and an option for some handlers too ? Cheers, Christophe

…

-- Christophe Raffalli tél: +689 87 23 11 48 web: http://raffalli.eu

c-cube · 2021-12-12T17:14:36Z

Ah, very interesting, I wrote the code assuming http 1.1. Could be an option in create, or perhaps a little middleware that injects the proper headers :)

craff · 2021-12-12T18:59:23Z

You read the version in the current version with scanf and accept 1.0. If we want to obey the standard, we should close the connection if Http 1.0 and no keep-alive Or Http 1.1 and connection: close Otherwise we are assuming the client will client will do the job? Le 12 décembre 2021 07:14:46 GMT-10:00, Simon Cruanes ***@***.***> a écrit :

…

Ah, very interesting, I wrote the code assuming http 1.1. Could be an option in `create`, or perhaps a little middleware that injects the proper headers :) -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #23 (comment)

c-cube · 2021-12-12T21:38:19Z

that sounds like a good idea. Are you game to implement it? At least open an issue?

craff · 2021-12-12T22:13:29Z

Yes, sure. I committed to stop using my own small server. And I like light design with readable .mli ;-) I will look at middleware next. My project is a plateform for teaching with python in the browser.... And it's already in production after only two weeks of dev. Le 12 décembre 2021 11:38:29 GMT-10:00, Simon Cruanes ***@***.***> a écrit :

…

that sounds like a good idea. Are you game to implement it? At least open an issue? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #23 (comment)

c-cube · 2021-12-12T22:33:31Z

That sounds like a nice project :). You're teaching in Savoie, if I understand correctly?

craff · 2021-12-13T01:26:36Z

I am teaching in Tahiti, leaving on a boat! Le 12 décembre 2021 12:33:42 GMT-10:00, Simon Cruanes ***@***.***> a écrit :

…

That sounds like a nice project :). You're teaching in Savoie, if I understand correctly? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #23 (comment)

c-cube · 2021-12-13T01:40:52Z

Damn, that sounds lovely :)

craff added 3 commits December 8, 2021 11:46

Added a max_keep_alive and moved to a non blocking reading of request…

4c7d27d

… not to loose connection slot. Default is max_keep_alive = -1.0 which keeps the original behaviour.

Merge branch 'c-cube:master' into master

b394527

only set non blocking for reading request, not to write answer

9e9fe32

craff mentioned this pull request Dec 9, 2021

basic timeout on requests #5

Closed

c-cube reviewed Dec 9, 2021

View reviewed changes

craff and others added 3 commits December 8, 2021 16:25

Update src/Tiny_httpd.mli

024a774

Co-authored-by: Simon Cruanes <simon.cruanes.2007@m4x.org>

release semaphore does not return anything anymore

e87428a

Merge branch 'master' of github.com:craff/tiny_httpd

8da6967

craff mentioned this pull request Dec 9, 2021

Provide some level of statistics ? #25

Closed

c-cube requested changes Dec 9, 2021

View reviewed changes

src/Tiny_httpd.ml Outdated Show resolved Hide resolved

remove mutex to get available_connections

9b4a1b1

make accessible to the end user the number of available connections

97eeaf9

c-cube reviewed Dec 10, 2021

View reviewed changes

moved back acquire before accept but inside the try ... with ... one …

c3265af

…never knowns

c-cube merged commit 6dcae7f into c-cube:master Dec 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

max_keep_alive and non blocking reading of request #23

max_keep_alive and non blocking reading of request #23

craff commented Dec 8, 2021

c-cube left a comment

c-cube Dec 9, 2021

craff Dec 9, 2021

c-cube Dec 9, 2021

craff Dec 9, 2021

c-cube Dec 9, 2021

craff Dec 9, 2021

c-cube left a comment

craff commented Dec 9, 2021

craff commented Dec 9, 2021

craff commented Dec 9, 2021

c-cube Dec 10, 2021

craff Dec 10, 2021

c-cube Dec 11, 2021

craff commented Dec 10, 2021

c-cube commented Dec 11, 2021

craff commented Dec 11, 2021 via email

craff commented Dec 11, 2021 via email

c-cube commented Dec 11, 2021

craff commented Dec 12, 2021 via email

craff commented Dec 12, 2021 via email

c-cube commented Dec 12, 2021

craff commented Dec 12, 2021 via email

c-cube commented Dec 12, 2021

craff commented Dec 12, 2021 via email

c-cube commented Dec 12, 2021

craff commented Dec 13, 2021 via email

c-cube commented Dec 13, 2021

max_keep_alive and non blocking reading of request #23

max_keep_alive and non blocking reading of request #23

Conversation

craff commented Dec 8, 2021

c-cube left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-cube left a comment

Choose a reason for hiding this comment

craff commented Dec 9, 2021

craff commented Dec 9, 2021

craff commented Dec 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craff commented Dec 10, 2021

c-cube commented Dec 11, 2021

craff commented Dec 11, 2021 via email

craff commented Dec 11, 2021 via email

c-cube commented Dec 11, 2021

craff commented Dec 12, 2021 via email

craff commented Dec 12, 2021 via email

c-cube commented Dec 12, 2021

craff commented Dec 12, 2021 via email

c-cube commented Dec 12, 2021

craff commented Dec 12, 2021 via email

c-cube commented Dec 12, 2021

craff commented Dec 13, 2021 via email

c-cube commented Dec 13, 2021