net: add run_server to run eio servers

[bikallem]

This PR adds run_server - to be used to spawn network socket server. The current version supports spawning both single domain and multi domain servers. The default is a single domain concurrent server.

Based on discussion in mirage/ocaml-cohttp#961 (comment).

/cc @talex5 @patricoferris

[bikallem]

The ~additional_domains bit doesn't work with luv backend. Should this be reflected in the API doc?

[bikallem]

@polytypic Thank you for the review. I have pushed some changes that should address the raised issues.

[bikallem]

@talex5 I believe I have addressed all of your points. Could we include this PR in the next release please.

[polytypic]

LGTM

[talex5]

I see you scaled back the tests since last time. I'm putting them back (using mocks) and fixing up some bugs that uncovered...

[bikallem]

I see you scaled back the tests since last time.

Yes, I was getting some error while trying to use the mocks. So I scaled it to testing just one client connection for now.

I'm putting them back (using mocks) and fixing up some bugs that uncovered..

That would be nice indeed.

[talex5]

I pushed my suggested changes here: https://github.com/talex5/eio/commits/run_server_multicore

• Added mock tests for single and multi-domain cases, maximum connections, cancellation and graceful shutdown.
• I brought back your stop parameter - I agree that it's a nicer way to do it.
• The documentation said additional_domains gives the number of additional domains, but the code used it as the total. I changed the code to match.
• I moved Semaphore.acquire outside of the protect. Otherwise failing to acquire it will do a release.
• I got it to release the semaphore when the connection handler finishes, not when it starts.
• Graceful shutdown previously only stopped the main domain. Now it stops all of them.
• I left the test for max_connections allowing 0. It's not exactly wrong, so I guess we should allow it.

[bikallem]

I pushed my suggested changes here: https://github.com/talex5/eio/commits/run_server_multicore

The changes look good to me, especially the tests are much more illustrative of the functionality of run_server.

One nit, I see that the documentation for @raise has been removed but we still raise Invalid_argument. Is this (the documentation) not required?

[bikallem]

@talex5 I have cherry-picked your commit to this PR and re-added the @raise documentation.

I left the test for max_connections allowing 0. It's not exactly wrong, so I guess we should allow it.

max_connections value at <=0 raises Invalid_argument as you suggested here (#408 (comment)).

[bikallem]

As an aside, Interestingly, Eio.Semaphore.make allows n=0. However, in such cases, how does Eio.Semaphore.acquire s work? It should never return right?

I just tried below in utop:

─( 13:55:02 )─< command 0 >─────────────────────────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # let m = Eio.Semaphore.make 0;;
val m : Eio.Semaphore.t = <abstr>
─( 13:55:02 )─< command 1 >─────────────────────────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # Eio.Semaphore.acquire;;
- : Eio.Semaphore.t -> unit = <fun>
─( 13:55:14 )─< command 2 >─────────────────────────────────────────────────────────────────────────────────────{ counter: 0 }─
utop # Eio.Semaphore.acquire m;;
Exception: Stdlib.Effect.Unhandled(Eio__core__Suspend.Suspend(_))

EDIT: See below comment, I seem to have missed the Eio_main.run bit.

[bikallem]

Yes, it does hang. See below the correct utop session below:

utop # Eio_main.run @@ fun env ->
let m = Eio.Semaphore.make 0 in
Eio.Semaphore.acquire m;;
....

The .... bit denotes the Eio.Semaphore.acquire m never returns. Should Eio.Semaphore.make n require n > 0?

[bikallem]

In order to not derail this PR and track the issue separately, I have opened an issue for Semaphore #415.

[polytypic]

Oh! I missed this during my review.

So, IIUC, this now creates a new switch that is different from the switch passed in.

I wonder whether there might be ways to make these kinds of programming errors with switches more difficult to make?

[polytypic]

Would it make sense to somehow explicitly pass some (parent) context for a switch? @talex5

[talex5]

This was due to my (bad) suggestion in #408 (comment). In my branch I instead replaced this with a single switch and a promise (as @bikallem had it originally), which avoids the confusion.

[polytypic]

Well, I was basically suggesting the same (use cancellation) in one of my comments.

[talex5]

One nit, I see that the documentation for @raise has been removed but we still raise Invalid_argument. Is this (the documentation) not required?

I often don't document Invalid_argument exceptions, as we don't (necessarily) want to promise to raise them, and we don't expect people to catch them. Usually I prefer to say that you have to ensure domains >= 0 when calling it, for example (but it seemed obvious in this case). But there's also no harm in documenting this here I think.

I left the test for max_connections allowing 0. It's not exactly wrong, so I guess we should allow it.

max_connections value at <=0 raises Invalid_argument as you suggested here (#408 (comment)).

OK; I don't have very strong opinions about this.

[polytypic]

• I brought back your stop parameter - I agree that it's a nicer way to do it.

@talex5 Hmm... I kind of find this unfortunate as being able to generally rely on cancellation sounds nice in principle. At any rate, it might be nice to have some "standard" way for servers to allow them to be stopped. One might have applications running multiple servers for different purposes/types of connections. So, some sort of standard shutdown mechanism could be nice.

[bikallem]

I often don't document Invalid_argument exceptions, as we don't (necessarily) want to promise to raise them, and we don't expect people to catch them. Usually I prefer to say that you have to ensure domains >= 0 when calling it, for example (but it seemed obvious in this case). But there's also no harm in documenting this here I think.

@talex5 Yes, same here. IIUC Invalid_argument usually (mostly?) means programmer error rather than a runtime error that user should handle. However, stdlib seems to document it quite a bit (https://github.com/ocaml/ocaml/blob/trunk/stdlib/string.mli#L88) so I thought it was a good practice to follow. For future reference, do you prefer this not being documented? I don't mind either but want to be consistent.

[talex5]

I brought back your stop parameter - I agree that it's a nicer way to do it.

@talex5 Hmm... I kind of find this unfortunate as being able to generally rely on cancellation sounds nice in principle.

You can rely on cancellation to stop the server and all connections. That is useful if something failed and you want to stop everything and report the error. But graceful shutdown seems a bit different, as it's not an error. So you have to take the shutdown request, create an exception for it, use that to cancel, then catch the exception again and continue.

Everything needs to support cancellation by default because a crash might happen anywhere in a program and you don't want to be stuck waiting for an uncancellable operation before you can report the error. But graceful shutdown is something you need to support explicitly anyway.

At any rate, it might be nice to have some "standard" way for servers to allow them to be stopped.

I expect this ?stop argument will be the standard way. It's already used in some other projects, e.g. Dream.run has ?stop:unit promise. I used 'a Promise.t though to make it clear that the server only finishes by stopping, and if you don't pass ?stop then it never returns.

[bikallem]

Thanks.