eio icon indicating copy to clipboard operation
eio copied to clipboard

net: add somaxconn to Eio.Net

Open bikallem opened this issue 1 year ago • 2 comments

bikallem avatar Sep 13 '22 17:09 bikallem

Can you clarify why we need this ?

Most systems will just silently cap if you go over the max, so if this is important they can just pass a big number to listen, it also doesn't have a huge implication on performance, if it does we're not calling listen fast enough and we should lose connections anyway.

haesbaert avatar Sep 23 '22 19:09 haesbaert

Mostly following lwt here (https://github.com/ocsigen/lwt/blob/master/src/unix/lwt_io.ml#L1604) as I am looking at the eio version of establish_server.

Most systems will just silently cap if you go over the max, so if this is important they can just pass a big number to listen, it

Yes. This PR attempts to be accurate with the "big number" by default so the user of eio doesn't have to guess.

bikallem avatar Sep 24 '22 10:09 bikallem

Ci is failing due to OCaml 5 issue I think - (https://ci.ocamllabs.io/github/ocaml-multicore/eio/commit/619f75f961e27a897ab37a5b433688d01c38feb8/variant/(lint-doc)).

@talex5 any suggestions on getting the CI green?

bikallem avatar Nov 10 '22 17:11 bikallem

I'm not sure defaulting to a large number is even useful. https://serverfault.com/a/519152 says:

Sometimes it's preferable to fail fast and let the load-balancer to do it's job(retry) than to make user wait - for that purpose we set net.core.somaxconn any value, and limit application backlog to e.g. 10 and set net.ipv4.tcp_abort_on_overflow to 1.

https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-listen says:

When calling the listen function in a Bluetooth application, it is strongly recommended that a much lower value be used for the backlog parameter (typically 2 to 4), since only a few client connections are accepted. This reduces the system resources that are allocated for use by the listening socket. This same recommendation applies to other network applications that expect only a few client connections.

So probably better to make the application choose a value.

talex5 avatar Nov 17 '22 12:11 talex5

yes please, don't default to max, I think forcing the user to pass the value was fine.

haesbaert avatar Nov 17 '22 12:11 haesbaert

@talex5 @haesbaert I have removed somaxconn being the default value for listen ~backlog now.

bikallem avatar Nov 18 '22 10:11 bikallem

@haesbaert @talex5 If there are no objections could you please approve/merge this PR.

bikallem avatar Dec 23 '22 16:12 bikallem

If there are no objections could you please approve/merge this PR.

But what problem is this solving? So far the only explanation is "Mostly following lwt here", which doesn't seem very convincing. I suggest closing this until we find a use for it.

talex5 avatar Jan 06 '23 14:01 talex5

Yes, there is no direct or pressing need for this PR; so closing this now.

bikallem avatar Jan 06 '23 16:01 bikallem