Implement Eio.Resource.Close

[paurkedal]

After shutting down the TLS flow, I would like to close the underlying TCP socket. This is of course handled by the switch, but since I'm using resource pooling, I would like to do this before leaving the scope of the switch. I could pass around the TCP flow along with the TLS flow, but this breaks the flow-abstraction which is very useful when dealing with TLS.

My proposal here is the the TLS flow implements the close interface but falls back to no-op if the underlying flow does not implement it. We could consider making close a requirement for the underlying flow, since it should be possible to amend a no-op close operation to an existing flow in the (unlikely?) event that a non-TCP flow is used.

(I made a hack around the issue, but I prefer a more proper solution.)

[hannesm]

First of all thanks for your contribution :). As I don't understand eio and its desired semantics, I'd be happy if someone with more clue @talex5 @bikallem would review this PR.

[talex5]

It's up to tls-eio to decide whether closing a TLS flow also closes the underlying flow.

For tls-lwt, we have:

(** [of_t t] is [ic, oc], the input and output channel.  [close]
    defaults to [!Unix.close]. *)
val of_t : ?close:(unit -> unit Lwt.t) -> Unix.t -> ic * oc

So, tls-lwt closes by default but you can override it.

tls-mirage closes in all cases.

I find it a bit ugly that this PR uses runtime introspection to find out whether it can close the underlying flow. Since it's probably only useful in rare cases, we could just default to not closing.

[paurkedal]

@talex5 Thanks for the input, I'm new to EIO and still learning, as well.

I agree the runtime introspection is ugly, and I changed my mind about the PR in the current form. I think that the best solution is to require that the underlying flow implements the close operation. I would expect close to be present for TCP flows, or any flows where TLS is relevant for protecting the data. But if I'm wrong, there could be code out there which breaks after the change, and which will need a noop close amended.

Would it not also be best to close by default in consistency with tls-lwt? I can't think of cases where it would be relevant to re-use the underlying flow after the TLS session is complete. Of course, when using switches either way works, but releasing resources as soon as possible is a good thing.

[talex5]

I think that the best solution is to require that the underlying flow implements the close operation.

Sounds good to me!

(it would also be possible to use a polymorphic type to remember whether the underlying flow is closable, but I suspect that would be more trouble than it's worth and would certainly break existing code)

[paurkedal]

I've updated the PR to require close from the socket. If we want to make it optional, I could add either a boolean parameter or a function like for tls-lwt.

[talex5]

Looks good to me. CI is failing for an unrelated reason:

File "lwt/examples/resume_echo_server.ml", line 55, characters 35-61:
55 |     obfuscation = Randomconv.int32 Mirage_crypto_rng.generate ;
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This expression has type int -> HT.key
       but an expression was expected of type int -> string
       Type HT.key is not compatible with type string

[hannesm]

Ooops sorry, I forgot about this PR. If you could rebase to the main branch, and @talex5 approval still stands, I can merge and cut a release for tls-eio.

[paurkedal]

No problem, I pushed a branch rebased on main. Let me know if you prefer it squashed too.

[hannesm]

thanks @paurkedal, merged and I re-did the 0.17.4 release to include this PR. ocaml/opam-repository#25587