Close no err

[hannesm]

A subtle issue, which I don't know how to fix properly: when read or write fail, what is the suggested semantics of the socket? should it be closed or am I responsible of closing it? since on Unix, T.close can Lwt.fail as well (i.e. if the fd is already closed), but Mirage_flow.TCP.close does only specify unit io as return value, I don't know how that should be handled. For now, I had to Lwt.catch (fun () -> T.close flow) (fun _ -> Lwt.return_unit), so:

• is this the intended behaviour (if so, we should document this in T.close)
• should close signature be able to return (unit, err) result io?
• should close just close and ignore potential issues? (as a side note, close is one of the trickiest system calls, please read up on https://discuss.ocaml.org/t/system-calls-unix-close-signals-eintr/972)

-- this is an implementation of the last option, please raise your concern if you disagree

NB: AFAIU the direct implementation, there close will never Lwt.fail (neither able to Error out, but it does some Lwt.sequence magic I don't understand)

[hannesm]

(only the last commit in here is relevant, the other is in #364, but git told me it can't rebase that on master)

[hannesm]

to add some beef here, when using irmin 1.3.2, irmin-git 1.3.0, irmin-mirage 1.3.0, conduit 1.1.0 mirage-conduit 3.0.1 and a simple git clone thingy https://github.com/hannesm/git-test , I get an error when compiling for unix (see master.out), whereas with the commit from this PR, I can clone successfully (see close-no-err.out) at https://gist.github.com/hannesm/334e24aa7204c5b1d41b7d11cd6968ee (/cc @samoht).

I can clone fine with ukvm (instead of unix) backend and the very same unikernel using either branch of tcpip, or the latest release. also when using the direct instead of the socket stack on unix, it works fine with either branch.

[samoht]

I'm fine with the 3rd implementation (e.g. just ignore potential errors on close). But your pattern is a bit too generic: please just catch Unix.Unix_error _ errors (and for close it could just be EBADF normally).

My only worry was the same as @djs55 made somewhere else (I can't find the reference anymore, sorry) is that our use of file descriptors could be unsafe and some races in the underlying OS could cause us to close/reuse a existing fd number. But I have just checked the code of lwt_unix and I think we are safe: the state of the fd is kept in an internal mutable field, and EBADF exception is raised on double-close, before trying to close the fd again.

Also can you rebase now that #364 is merged? Thanks!

[hannesm]

I rebased and only ignore EBADF now (re-raising other errors).

[hannesm]

@samoht regarding @djs55 mention about close and race condition, maybe you mean this comment #347 (comment) ? as discussed in the linked discourse thread, what should we do when close returns EINTR in a concurrent setting (since there's no indication whether the fd was closed or not), I bet the safe thing is to not call close again, but rather risk a fd leakage. In implementations of close (e.g. on linux), it seems that EINTR is never returned (which is pretty sane behaviour).

[hannesm]

does this "approve" imply I should merge this myself? and once merged, should I attempt to prepare a new release of tcpip with the various fixes for the socket-stack? (or is anyone else terribly motivated to draft a release, or anyone aware of showstoppers we should fix before a new release)?

//cc @mirage/core

EDIT: in the meantime I found moby/vpnkit#389 and am curious whether the keepalive code should be fixed / reverted in mirage-tcpip before a new release!? //cc @djs55

[djs55]

I completely agree that risking an fd leak is the best thing to do ... and hopefully this never happens in practice.

Regarding the keep alive memory leak: there is a bug in the implementation where the connection state is kept alive until the final timer expires, even if the stack receives a RST from the peer. This did cause a memory leak in vpnkit. The leak only happens if the keep-alive behaviour is explicitly enabled, so it's safe for our existing code. Since it involved interfaces changes across this repo and mirage-protocols (and I think the interface is ok) I'd rather not revert it entirely if we can avoid it. How about something like

• release as-is, and document that the keep-alive shouldn't be used for now; or
• add a patch to raise a fatal exception if the keep-alive is enabled

I don't think there are any other big problems that need fixing :)

(I also need to fix the keep-alive bug but I don't want to rush it -- it turns out to be a bit more tricky than I first anticipated)

[djs55]

(BTW I reckon that you should feel free to merge now that @samoht has hit the "Approve" button)

[hannesm]

@djs55 thanks for your comment. I'm all for documenting the known issues of keepalive, mainly to prevent others of running in this known problem. in regards to raising an exception if keepalive is enabled, I'm really unsure: does the keepalive code always lead to a memory leak, or only in certain circumstances? i.e. is there a chance of using keep alive without running into this issue?

it's safe for our existing code

I'm not sure what the maintenance guarantee for mirage-tcpip is - since there are several releases already with keepalive support (namely 3.4.0 and 3.4.1), there may be already third parties using these features - in public or private repositories somewhere.

I'm not a maintainer of this repository, but would suggest to:

• (a) document and issue a compile-time warning (if possible, is there some generic mechanism for @@deprecated?) when the memory-leaking code path is entered (if this is not possible, raise a run-time exception instead)
• (b) mark 3.4.0 and 3.4.1 unavailable once a 3.4.2 is released with the above mentioned semantics
• (c) or generate a pkg-message with regard to keepalive and earlier releases -- but opam's pkg-message are easy to ignore (I never read them)
• (d) this way (a) and (b), clients of tcp are informed that the keepalive feature is not yet ready for production

[djs55]

I believe it's possible to use keep-alive safely despite the bug in some circumstances -- the memory is eventually freed when the final timer expires. On a busy system with a high rate of TCP connections and a long timeout value the heap can grow unexpectedly large (perceived as a leak, in practice possibly indistinguishable from a leak if the system grinds to a halt or crashes, even if technically the memory will eventually be returned). If the system is only handling a small number of infrequent connections with a short timeout value then it should behave fairly normally.

I'll make a start by creating an issue here with a write up / impact assessment.

Thanks for your other suggestions, I'll investigate those.

[hannesm]

@djs55 thanks! I'll merge this PR now since @samoht and @djs55 agree that it is fine. would be great if someone, after the keep-alive stuff is documented, would draft a new release. thanks!