ocaml-multicore / eio

Effects-based direct-style IO for multicore OCaml
Other
548 stars 66 forks source link

Problematic `assert()` in `Eio.File.pread` #579

Closed SGrondin closed 1 year ago

SGrondin commented 1 year ago

I might be wrong, but this assert appears misguided to me.

(* Current implementation of Eio.File.pread: *)

let pread (t : #ro) ~file_offset bufs =
  let got = t#pread ~file_offset bufs in
  assert (got > 0 && got <= Cstruct.lenv bufs);
  got

Imagine I'm reading a file slice by slice, using successive calls to Eio.File.pread.

When I reach the end of the file (i.e. when I pass a ~file_offset equal to the number of bytes in the file), Eio.File.pread returns a length of 0 and thus the assert fails.

It appears to me that the author expected t#pread to raise End_of_file to indicate EOF.

Either the Eio_posix implementation of Eio.File.ro#pread needs to raise End_of_file, or the assertion should be

assert (got <= Cstruct.lenv bufs);

I might be wrong here! but at the moment I'm having to circumvent Eio.File.pread by making direct calls to t#pread.

talex5 commented 1 year ago

Yes, eio_posix needs to change (#581). The assert is here precisely to make sure that all the backends behave the same way (eio_linux already raises End_of_file here).