Faraday in Eio (some bugs)

[talex5]

Hi, I needed a buffered writer in Eio, and I used Faraday as the basis of that (ocaml-multicore/eio#235). While reviewing the code, I fixed a few things that might want fixing here too.

Overflow

There is a comment in shift_flushes saying that it avoids overflow problems by subtracting both sides:

faraday/lib/faraday.ml

Lines 392 to 410 in 3ea082b

	| (threshold, f) as flush ->
	(* Edited notes from @dinosaure:
	*
	* The quantities [t.bytes_written] and [threshold] are always going to be
	* positive integers. Therefore, we can treat them as unsinged integers for
	* the purposes of comparision. Doing so allows us to handle overflows in
	* either quantity as long as they're both within one overflow of each other.
	* We can accomplish this by subracting [min_int] from both quantities before
	* comparision. This shift a quantity that has not overflowed into the
	* negative integer range while shifting a quantity that has overflow into
	* the positive integer range.
	*
	* This effectively restablishes the relative difference when an overflow
	* has occurred, and otherwise just compares numbers that haven't
	* overflowed as similarly, just shifted down a bit.
	*)
	if t.bytes_written - min_int >= threshold - min_int
	then begin f reason; shift_flushes t ~reason end
	else Flushes.enqueue_front flush t.flushed

This doesn't work; it just shifts the problem elsewhere (consider bytes_written = -2 and threshold = 8, for example). I believe the test should be t.bytes_written - threshold >= 0. /cc @dinosaure

Safety

write_string and write_bytes both perform unsafe blits using offsets provided by the user:

faraday/lib/faraday.ml

Lines 250 to 258 in 3ea082b

	let write_string =
	let length = String.length in
	let blit = Bigstringaf.unsafe_blit_from_string in
	fun t ?off ?len a -> write_gen t ~length ~blit ?off ?len a
	let write_bytes =
	let length = Bytes.length in
	let blit = Bigstringaf.unsafe_blit_from_bytes in
	fun t ?off ?len a -> write_gen t ~length ~blit ?off ?len a

Serialize loop

If the user's writev function returns Closed then it can't consume anything and just calls it again, forever:

faraday/lib/faraday.ml

Lines 436 to 444 in 3ea082b

	let rec serialize t writev =
	match operation t with
	| `Writev iovecs ->
	begin match writev iovecs with
	| `Ok n -> shift t n; if not (Buffers.is_empty t.scheduled) then yield t
	| `Closed -> close t
	end;
	serialize t writev
	| (`Close|`Yield) as next -> next

License

BTW, Faraday uses the BSD-3-clause license, whereas Eio uses ISC. I included your headers and license in Eio, but if you're willing to let us use it as ISC it would simplify things slightly.

[dinosaure]

This doesn't work; it just shifts the problem elsewhere (consider bytes_written = -2 and threshold = 8, for example). I believe the test should be t.bytes_written - threshold >= 0. /cc @dinosaure

The comment is probably not clear. The code does not wants to fix integer overflow but it wants to shift the problem more far than 0x3FFFFFFFFFFFFFFF and consider these integers as unsigned integer. With min_int, the comparison should work until 0x7FFFFFFFFFFFFFFF. This is the same trick used by the OCaml standard library here. So you are true, the integer overflow remains in anyway.

[talex5]

I don't see much point in just doubling the time until it fails. On 32-bit systems, it means that instead of failing after writing 1GB of data, it will instead fail after writing 2 GB.

For 64-bit systems, it doesn't matter, of course.

[dinosaure]

I don't see much point in just doubling the time until it fails. On 32-bit systems, it means that instead of failing after writing 1GB of data, it will instead fail after writing 2 GB.

It's my little stone to the building 🙂