Faraday in Eio (some bugs)

[talex5]

Hi, I needed a buffered writer in Eio, and I used Faraday as the basis of that (https://github.com/ocaml-multicore/eio/pull/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:

https://github.com/inhabitedtype/faraday/blob/3ea082b23b7ffef71a19f29b216d9c23d1f33c47/lib/faraday.ml#L392-L410

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:

https://github.com/inhabitedtype/faraday/blob/3ea082b23b7ffef71a19f29b216d9c23d1f33c47/lib/faraday.ml#L250-L258

Serialize loop

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

https://github.com/inhabitedtype/faraday/blob/3ea082b23b7ffef71a19f29b216d9c23d1f33c47/lib/faraday.ml#L436-L444

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 🙂