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.

lwt/src/unix/lwt_unix.cppo.mli

Lines 377 to 422 in de3257e

	(** Sequences of buffer slices for {!writev}. *)
	module IO_vectors :
	sig
	type t
	(** Mutable sequences of I/O vectors. An I/O vector describes a slice of a
	[bytes] or [Bigarray] buffer. Each I/O vector is a triple containing a
	reference to the buffer, an offset into the buffer where the slice begins,
	and the length of the slice. *)
	type _bigarray =
	(char, Bigarray.int8_unsigned_elt, Bigarray.c_layout) Bigarray.Array1.t
	(** Type abbreviation equivalent to {!Lwt_bytes.t}. Do not use this type name
	directly; use {!Lwt_bytes.t} instead. *)
	val create : unit -> t
	(** Creates an empty I/O vector sequence. *)
	val append_bytes : t -> bytes -> int -> int -> unit
	(** [append_bytes vs buffer offset length] appends a slice of the [bytes]
	buffer [buffer] beginning at [offset] and with length [length] to the
	I/O vector sequence [vs]. *)
	val append_bigarray : t -> _bigarray -> int -> int -> unit
	(** [append_bigarray vs buffer offset length] appends a slice of the
	[Bigarray] buffer [buffer] beginning at [offset] and with length [length]
	to the I/O vector sequence [vs]. *)
	val drop : t -> int -> unit
	(** [drop vs n] adjusts the I/O vector sequence [vs] so that it no longer
	includes its first [n] bytes. *)
	val is_empty : t -> bool
	(** [is_empty vs] is [true] if and only if [vs] has no I/O vectors, or all I/O
	vectors in [vs] have zero bytes. *)
	val system_limit : int option
	(** Some systems limit the number of I/O vectors that can be passed in a
	single call to their [writev] or [readv] system calls. On those systems,
	if the limit is [n], this value is equal to [Some n]. On systems without
	such a limit, the value is equal to [None].
	Unless you need atomic I/O operations, you can ignore this limit. The Lwt
	binding automatically respects it internally. See {!Lwt_unix.writev}.
	A typical limit is 1024 vectors. *)
	end

[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

lwt/src/unix/lwt_unix.cppo.mli

Lines 921 to 925 in 4e592eb

	type io_vector = {
	iov_buffer : string;
	iov_offset : int;
	iov_length : int;
	}

they take the newer Lwt_unix.IO_vectors.t

lwt/src/unix/lwt_unix.cppo.mli

Lines 321 to 325 in 4e592eb

	type t
	(** Mutable sequences of I/O vectors. An I/O vector describes a slice of a
	[bytes] or [Bigarray] buffer. Each I/O vector is a triple containing a
	reference to the buffer, an offset into the buffer where the slice begins,
	and the length of the slice. *)

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 ***@***.***> 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: #594 (comment)

-- 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

lwt/src/unix/lwt_unix.cppo.mli

Lines 930 to 933 in 705a206

	val recv_msg : socket : file_descr -> io_vectors : io_vector list -> (int * Unix.file_descr list) Lwt.t
	[@@ocaml.deprecated
	" Will be replaced by Lwt_unix.Versioned.recv_msg_2 in Lwt >= 5.0.0. See
	https://github.com/ocsigen/lwt/issues/594"]

now by calls to Lwt_unix.Versioned.recv_msg_2:

lwt/src/unix/lwt_unix.cppo.mli

Lines 1505 to 1507 in 705a206

	val recv_msg_2 :
	socket:file_descr -> io_vectors:IO_vectors.t ->
	(int * Unix.file_descr list) Lwt.t

...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.