Add API for seekable read/writes by nojb · Pull Request #307 · ocaml-multicore/eio

nojb · 2022-09-04T18:39:36Z

This partly addresses #305. Currently just a draft, meant for discussing the API.

The PR adds a method #read_at_offset to the object returned by open_in and with_open_in allowing to do a read at a given offset. The new method is exposed as Eio.Fs.read_at_offset and a helper function is added as well Eio.Fs.read_exact_at_offset. Not sure if this is the right level at which to expose this functionality.

I'm just getting started with this library and am not sure I have yet grokked the full picture, so all feedback is welcome!

talex5

Thanks! I added some suggestions inline (I'm not quite sure what the best API would be either).

talex5 · 2022-09-07T10:00:14Z

lib_eio/fs.ml

 exception Not_found of path * exn
 exception Permission_denied of path * exn

+class virtual read_at_offset = object


Might as well make a ro type for this and related operations. We already have rw type.

Suggested change

class virtual read_at_offset = object

class virtual ro = object (_ : <Generic.t; Flow.source; ..>)

Thanks, done.

talex5 · 2022-09-07T10:00:43Z

lib_eio/fs.ml

 class virtual dir = object (_ : #Generic.t)
  method probe _ = None
-  method virtual open_in : sw:Switch.t -> path -> <Flow.source; Flow.close>
+  method virtual open_in : sw:Switch.t -> path -> <Flow.source; Flow.close; read_at_offset>


Suggested change

method virtual open_in : sw:Switch.t -> path -> <Flow.source; Flow.close; read_at_offset>

method virtual open_in : sw:Switch.t -> path -> <ro; Flow.close>

talex5 · 2022-09-07T10:06:27Z

lib_eio/fs.ml

 exception Permission_denied of path * exn

+class virtual read_at_offset = object
+  method virtual read_at_offset : int64 -> Cstruct.t -> int


I don't particularly like the name, but POSIX and Lwt_unix both call this pread.

Elsewhere, we're using Int63.t as the type for offsets, to avoid an allocation.

I renamed to pread, and switched to Int63.t.

talex5 · 2022-09-07T10:07:14Z

lib_eio/fs.ml

+  method virtual read_at_offset : int64 -> Cstruct.t -> int
+end
+
 class virtual rw = object (_ : <Generic.t; Flow.source; Flow.sink; ..>)


Suggested change

class virtual rw = object (_ : <Generic.t; Flow.source; Flow.sink; ..>)

class virtual rw = object (_ : <ro; Flow.sink; ..>)

talex5 · 2022-09-07T10:10:16Z

lib_eio/fs.ml

  method virtual close : unit
 end
+
+let read_at_offset (t : #read_at_offset) ofs buf =


Could just call this read and take an optional argument. eio_linux.mli uses file_offset, e.g.

Suggested change

let read_at_offset (t : #read_at_offset) ofs buf =

let read (t : #ro) ?file_offset buf =

Here I was in two minds because there already is a Flow.read which corresponds to the case of file_offset = None. So for the moment I renamed this to pread, with a mandatory ~file_offset argument (to avoid duplicating the functionality of Flow.read which may be a bit confusing).

nojb · 2022-09-10T14:58:57Z

Thanks for the review @talex5. I amended the PR roughly as suggested, and added an analogous "pwrite" API. Let me know what you think. Thanks!

talex5

Looks good to me! Can be merged once squashed and no longer marked as draft.

Two things I wonder about:

I see the linux and luv backends both accept a list of buffers. Maybe that should be exposed to users in the cross-platform API too?
Both backend APIs use write and write_single, whereas the new cross-platform API uses write_exact and write. (OCaml itself has Unix.single_write, but I dislike that word order because it's hard to find when using tab-completion).

nojb · 2022-09-13T07:25:55Z

I see the linux and luv backends both accept a list of buffers. Maybe that should be exposed to users in the cross-platform API too?

Makes sense, will amend.

Both backend APIs use write and write_single, whereas the new cross-platform API uses write_exact and write. (OCaml itself has Unix.single_write, but I dislike that word order because it's hard to find when using tab-completion).

Since read is already used by flow I kept the p prefix: pread and pwrite. Does pread_single/pread and pwrite_single/pwrite sound good for the new functions?

talex5 · 2022-09-13T14:13:42Z

Since read is already used by flow I kept the p prefix: pread and pwrite. Does pread_single/pread and pwrite_single/pwrite sound good for the new functions?

I'm a bit nervous about pread. Short reads are useful at the end of a file and it might be unexpected if that's an error. Perhaps it's best to be explicit and have p{read,write}_{all,single}.

nojb · 2022-09-13T15:48:12Z

Since read is already used by flow I kept the p prefix: pread and pwrite. Does pread_single/pread and pwrite_single/pwrite sound good for the new functions?

I'm a bit nervous about pread. Short reads are useful at the end of a file and it might be unexpected if that's an error. Perhaps it's best to be explicit and have p{read,write}_{all,single}.

Sorry, FWIW I just remembered why I had used pread and pread_exact originally: it matches the terminology in flow's read and read_exact.

Anyway, I don't have a strong opinion either way: pread/pread_exact or pread_single/pread_all. Let me know which one strikes you as preferable...

talex5 · 2022-09-14T17:29:35Z

Sorry, FWIW I just remembered why I had used pread and pread_exact originally: it matches the terminology in flow's read and read_exact.

That seems reasonable. Let's use pread/pread_exact for now then, and we can rename to p?read_single in both places in a future PR, if we decide that's better.

Anyway, I don't have a strong opinion either way: pread/pread_exact or pread_single/pread_all. Let me know which one strikes you as preferable...

pread_all sounds like it might mean to read all of the file.

nojb · 2022-09-14T17:47:48Z

That seems reasonable.

Perfect, I squashed the PR, and exposed the possibility of passing a list of buffers, as suggested. This is ready for merging on my side. Thanks!

Also, use `pwrite_single` and `pwrite_all` for clarity, and don't treat a 0-length write as `End_of_file`.

talex5

I pushed a commit with the suggested changes.

I also moved the file operations out to a new Eio.File module, added some doc-strings, and renamed the write operations slightly.

If you're happy with that, it can be merged.

talex5 · 2022-09-15T15:58:43Z

lib_eio_luv/eio_luv.ml

+    match File.write_single ~file_offset fd bufs |> or_raise |> Unsigned.Size_t.to_int with
+    | 0 -> raise End_of_file
+    | got -> got


Good catch.

talex5 · 2022-09-15T15:59:45Z

lib_eio/fs.ml

  method probe _ = None
  method read_methods = []


I guess it would make more sense to move these to ro, since they are about reading.

nojb · 2022-09-16T19:14:30Z

If you're happy with that, it can be merged.

LGTM, thanks.

haesbaert · 2022-09-16T19:41:47Z

For consistency, File.write is binding the result with or_raise (t -> Luv.Buffer.t list -> unit), shouldn't we make File.write_single the same ?

For consistency with the rest of the API. Spotted by Christiano Haesbaert.

talex5 · 2022-09-17T15:23:38Z

For consistency, File.write is binding the result with or_raise (t -> Luv.Buffer.t list -> unit), shouldn't we make File.write_single the same ?

Good point. I pushed a commit making write return a result, to match the rest of the API, and also exposed write_single.

@anmonteiro

CHANGES: Changes: - Update to OCaml 5.0.0~beta1 (@anmonteiro @talex5 ocaml-multicore/eio#346). - Add API for seekable read/writes (@nojb ocaml-multicore/eio#307). - Add `Flow.write` (@haesbaert ocaml-multicore/eio#318). This provides an optimised alternative to `copy` in the case where you are writing from a buffer. - Add `Net.with_tcp_connect` (@bikallem ocaml-multicore/eio#302). Convenience function for opening a TCP connection. - Add `Eio.Time.Timeout` (@talex5 ocaml-multicore/eio#320). Makes it easier to pass timeouts around. - Add `Eio_mock.Clock` (@talex5 ocaml-multicore/eio#328). Control time in tests. - Add `Buf_read.take_while1` and `skip_while1` (@bikallem ocaml-multicore/eio#309). These fail if no characters match. - Make the type parameter for `Promise.t` covariant (@anmonteiro ocaml-multicore/eio#300). - Move list functions into a dedicated submodule (@raphael-proust ocaml-multicore/eio#315). - Direct implementation of `Flow.source_string` (@c-cube ocaml-multicore/eio#317). Slightly faster. Bug fixes: - `Condition.broadcast` must interlock as well (@haesbaert ocaml-multicore/eio#324). - Split the reads into no more than 2^32-1 for luv (@haesbaert @talex5 @EduardoRFS ocaml-multicore/eio#343). Luv uses a 32 bit int for buffer sizes and wraps if the value passed is too big. - eio_luv: allow `Net.connect` to be cancelled (@talex5 @nojb ocaml-multicore/eio#311). - eio_main: Use dedicated log source (@anmonteiro ocaml-multicore/eio#326). - linux_eio: fix kernel version number in log message (@talex5 @nojb ocaml-multicore/eio#314). - Account for stack differences in the socketpair test (issue ocaml-multicore/eio#312) (@haesbaert ocaml-multicore/eio#313). Documentation: - Add Best Practices section to README (@talex5 ocaml-multicore/eio#299). - Documentation improvements (@talex5 ocaml-multicore/eio#295 ocaml-multicore/eio#337).

talex5 reviewed Sep 7, 2022

View reviewed changes

talex5 approved these changes Sep 12, 2022

View reviewed changes

nojb marked this pull request as ready for review September 14, 2022 17:42

Add Fs.pread, Fs.pread_exact, Fs.pwrite, Fs.pwrite_exact

831995c

Split file operations into new Eio.File module

4c8a764

Also, use `pwrite_single` and `pwrite_all` for clarity, and don't treat a 0-length write as `End_of_file`.

talex5 reviewed Sep 15, 2022

View reviewed changes

Make Eio_luv.Low_level.write return a result

5be0ec4

For consistency with the rest of the API. Spotted by Christiano Haesbaert.

talex5 merged commit 5a77787 into ocaml-multicore:main Sep 17, 2022

nojb deleted the seek branch September 17, 2022 16:32

talex5 mentioned this pull request Oct 12, 2022

[new release] eio_main, eio_luv, eio_linux and eio (0.6) ocaml/opam-repository#22247

Merged

	class virtual read_at_offset = object
	class virtual ro = object (_ : <Generic.t; Flow.source; ..>)

	method virtual open_in : sw:Switch.t -> path -> <Flow.source; Flow.close; read_at_offset>
	method virtual open_in : sw:Switch.t -> path -> <ro; Flow.close>

	class virtual rw = object (_ : <Generic.t; Flow.source; Flow.sink; ..>)
	class virtual rw = object (_ : <ro; Flow.sink; ..>)

	let read_at_offset (t : #read_at_offset) ofs buf =
	let read (t : #ro) ?file_offset buf =

Conversation

nojb commented Sep 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb commented Sep 10, 2022

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

nojb commented Sep 13, 2022

Uh oh!

talex5 commented Sep 13, 2022

Uh oh!

nojb commented Sep 13, 2022

Uh oh!

talex5 commented Sep 14, 2022

Uh oh!

nojb commented Sep 14, 2022

Uh oh!

talex5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nojb commented Sep 16, 2022

Uh oh!

haesbaert commented Sep 16, 2022

Uh oh!

talex5 commented Sep 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nojb commented Sep 4, 2022 •

edited

Loading