Lwt: string/bytes (apparent?) inconsistency in Lwt_unix.io_vector and Lwt_unix.recv_msg

[mseri]

Lwtr_unix.io_vector carries strings, that since ocaml 4.06.0, default to immutable structures. However recv_msg will record the responses into those strings, treating them effectively as bytes.

I just had a brief look at the code, without entering into any details, but shouldn't we avoid considering strings as mutable buffers? It seems to me that this compiles fine only because at the c interface level bytes and string are interchangeable. I think it would be worth to break the compatibility and make these io_vectors use a bytes buffer instead of a string buffer.

[aantron]

Agreed. If we break this, we might want to go all the way and switch to the new I/O vectors. Haven't looked into the detailed implications of that, though. https://github.com/ocsigen/lwt/blob/de3257e54dbb0651935d395d205478b2b3852fc0/src/unix/lwt_unix.cppo.mli#L377-L422

[aantron]

I searched opam and GitHub, and found the following repos using Lwt_unix.send_msg and/or Lwt_unix.recv_msg:

• avsm/ocaml-workflow. I think this is abandoned.
• isakmp/ike
• let-def/ocamp
• ocaml-community/obus
• xapi-project/wsproxy
• xapi-project/xcp-idl

Of these, only obus and xcp-idl are in opam.

@avsm, @hannesm, @let-def, @Freyr666, @pmetzger, (and @mseri) what do you think about Lwt breaking Lwt_unix.send_msg and Lwt_unix.recv_msg in 5.0.0, so that instead of taking an Lwt_unix.io_vector list

https://github.com/ocsigen/lwt/blob/4e592eb90b721966f2f0e2f3c4efe8a4536c84fa/src/unix/lwt_unix.cppo.mli#L921-L925

they take the newer Lwt_unix.IO_vectors.t

https://github.com/ocsigen/lwt/blob/4e592eb90b721966f2f0e2f3c4efe8a4536c84fa/src/unix/lwt_unix.cppo.mli#L321-L325

which is used by Lwt_unix.writev?

We could create send_msg_2 and recv_msg_2, or use the Lwt_unix.Versioned module, but I'd like to just delete the old io_vectors eventually.

[hannesm]

fine with me, haven't worked on ike for quite some time (and will adapt once i find time to work on it again)..

[meadofpoetry]

@aantron Regarding obus it seems it's not hard to fix, so there should be no problems, I suppose.

[mseri]

@aantron for xapi it should not be a problem (ping also @lindig)

[let-def]

@aantron fine with me too.

[lindig]

The exposure in xapi-project to this function is minimal. I would support changing the API and to fix the clients. (ping @edwintorok)

[edwintorok]

I'd be happy to update the xapi related repos to the new API, thanks for the heads up.

On 6 August 2019 12:45:20 BST, Christian Lindig [e-mail] wrote:

The exposure in xapi-project to this function is minimal. I would
support changing the API and to fix the clients. (ping @edwintorok)

--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
https://github.com/ocsigen/lwt/issues/594#issuecomment-518634574

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[pmetzger]

I assume you're asking me about obus because it's in ocaml-community. I've not hacked on it before but it seems doable. @Freyr666 might have an opinion on that.

[aantron]

Thanks, all! I'll ping here again when there is some action that can be taken on this issue, with information. It will be after the 4.3.0 release (which you can ignore), and again after 5.0.0, so you don't have to track Lwt closely on your end :)

[aantron]

4.3.0 has just been released, preparing for making this breaking change in Lwt 5.0.0 in November 2019 (in three months). If you'd like a soft upgrade path, you can replace calls to Lwt_unix.recv_msg

https://github.com/ocsigen/lwt/blob/705a206ad64149be6cc93bb1447ce22fd14017ca/src/unix/lwt_unix.cppo.mli#L930-L933

now by calls to Lwt_unix.Versioned.recv_msg_2:

https://github.com/ocsigen/lwt/blob/705a206ad64149be6cc93bb1447ce22fd14017ca/src/unix/lwt_unix.cppo.mli#L1505-L1507

...and likewise for send_msg.

If you do so, add the constraint "lwt" {>= "4.3.0"}. If you do this within the next three months, you won't have to do anything at all when Lwt 5.0.0 comes out.

This is completely optional, and part of the reason for having the soft upgrade path is to be able to put a deprecation message, to notify users we may not be aware of.

In any case, when Lwt 5.0.0 is released, we will put the proper constraints on packages already in opam, so you don't have to worry about that.

I'll ping this issue again when 5.0.0 is out, and close it.

[aantron]

The attached commit switched the functions in Lwt_unix to the new ones in Lwt_unix.Versioned. It will be released in the coming days in Lwt 5.0.0.