introduce Lwt_io.establish_server'

[rgrinberg]

Exactly like Lwt_io.establish_server but allows the callback to access
the client's socket.

Fix #323

[aantron]

Looks pretty reasonable.

• I thought it would make more sense, and be more general, to give access to the actual socket, not just the peer name. That would allow the user to do whatever they want with it, including call Unix.getpeername, write their own shutdown procedure, etc. Right now, AFAIK, there is no way to get the Lwt_unix.file_descr from the Lwt_io.channels, and the new argument doesn't add that.
• Did you intend for this to replace the existing establish_server in a future release? I think that makes sense. If so, I will take care of the details. It's fine to commit it initially as establish_server'.

[c-cube]

I think this is neat, but the name is not that good (' is as uninformative as possible). What about establish_server_with_socket? It's not as if every char cost more…

[rgrinberg]

@aantron

• Good point. The only disadvantage I see here is that users that need the socket have to pay for it with a syscall. For comparison, async indeed allows you to retrieve the fd out of the writer/reader end of its Lwt_io.channel equivalent. Nevertheless, I think your proposal is correct anyway

• I did not intend this but perhaps it's a good idea. I don't really see the harm in keeping the old function as it doesn't have negative side effects. This is your call though

@c-cube

Yeah, definitely. I'll let Anton adjust the names

[aantron]

The only disadvantage I see here is that users that need the socket have to pay for it with a syscall.

True. It does seem that we should still pass the address directly to the callback. Maybe we should pass three argument then (address, fd, and pair of channels), or maybe the right thing to do is instead to add a Lwt_io.to_fd : _ Lwt_io.channel -> Lwt_unix.file_descr option. The option is, of course, because not every Lwt_io.to_channel wraps an fd. I suspect, however, that this is a non-trivial change, because, as I recall, the fd, for those channels that do wrap fds, is actually hidden in a closure, and not so readily accessible. Not sure if you would want to do that right now.

Another option is to pass the address and fd, and if the user wants to create channels, the user can do so from the fd. This is actually what I originally thought you meant to do in #323. The cost here is that Lwt_io currently performs some non-trivial close procedure, and the user would lose the implicit access to that.

These issues make me think that Lwt_io is headed for at least a partial redesign. I guess we can accept this PR in whatever currently-reasonable form, and not block it on any such bigger work. I'm actually now leaning towards only adding the address argument, as in the first commit, because I think we want to add that no matter what. What to do about the fd seems, to me, like some kind of bigger problem, which maybe it is not necessary or convenient to deal with right now.

Additional opinions are welcome :)

[rgrinberg]

@aantron I got rid of the last commit

[aantron]

So, I went with the name establish_server_with_client_address, unwieldy as it may be. I also rewrote the docs, though they will still need more improvement later. I would appreciate it if you took a glance at them – see the commit linked above.