Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eio_posix: fix pread at end-of-file #581

Merged
merged 1 commit into from
Jul 12, 2023
Merged

Conversation

talex5
Copy link
Collaborator

@talex5 talex5 commented Jul 11, 2023

Reported by @SGrondin.

Fixes #579.

@talex5 talex5 added the bug Something isn't working label Jul 11, 2023
Reported by Simon Grondin.
@@ -71,7 +71,11 @@ let of_fd fd = object (_ : <Eio_unix.Net.stream_socket; Eio.File.rw>)
method read_methods = []
method copy src = copy src fd

method pread ~file_offset bufs = Low_level.preadv ~file_offset fd (Array.of_list bufs)
method pread ~file_offset bufs =
let got = Low_level.preadv ~file_offset fd (Array.of_list bufs) in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array/list mismatch in the low-level/high-level interface leads to more allocations, but I can't see how to get rid of that easily given what the underlying syscall expects.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I wouldn't mind the high-level interface exposing array. That way I can choose whether to convert my list to an array or adapt my algorithm to use array.

Copy link
Contributor

@avsm avsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@talex5 talex5 merged commit 7251db6 into ocaml-multicore:main Jul 12, 2023
5 checks passed
@talex5 talex5 deleted the pread-eof branch July 12, 2023 09:50
talex5 added a commit to talex5/opam-repository that referenced this pull request Jul 12, 2023
CHANGES:

New features / API changes:

- Extend `Eio.Condition` API (@talex5 ocaml-multicore/eio#563).
  - `loop_no_mutex` is a simpler and more efficient way to way for a condition.
  - `register_immediate` allows integration with other IO libraries.

- Expose `Eio.Stdenv.backend_id` (@bord-o ocaml-multicore/eio#560, reviewed by @talex5).
  Useful in tests to report which backend is being used.

- Remove deprecated features (@talex5 ocaml-multicore/eio#552, reviewed by @avsm).
  These were all already marked as deprecated in v0.10 and are now gone completely:
  - `Fiber.fork_sub`
  - `Eio_unix.{FD,Ipaddr,socketpair,getnameinfo}`
  - `Eio_linux.{FD,get_fd,get_fd_opt}`
  - `Eio_posix.Low_level.Fd`

- Allow calling `close` more than once (@talex5 ocaml-multicore/eio#547, requested by @anmonteiro, reviewed by @patricoferris, @avsm).

- Add `close` to socket type (@talex5 ocaml-multicore/eio#549).
  Simplifies the type signatures a bit by avoiding having to mention this everywhere.

Bug fixes:

- Fix handling of empty path strings (@talex5 ocaml-multicore/eio#569, reported by @SGrondin).
  Using "" instead of "." in some places resulted in an error.

- eio_posix: fix update to watched FDs on cancel (@talex5 ocaml-multicore/eio#574, reported and reviewed by @quernd).
  Cancelling the last watcher of an FD didn't remove it from the set passed to `poll`,
  which could result in constant wake-ups.

- eio_posix: fix `pread` at end-of-file (@talex5 ocaml-multicore/eio#581, reported by @SGrondin).
  It tried to return 0 instead of `End_of_file`, triggering an assertion.

- eio_posix: don't reap non-Eio child processes (@talex5 ocaml-multicore/eio#562).
  This allows spawning processes with e.g. the stdlib or Lwt
  (but see ocaml-multicore/lwt_eio#19 for Lwt support).

- Preserve backtraces across `Domain_manager.run` (@talex5 ocaml-multicore/eio#571).
  See ocaml/ocaml#12362.

- Correct the backend selection for Cygwin (@dra27 ocaml-multicore/eio#557).
  Use `eio_posix`, not `eio_windows` in this case.

Other changes:

- Simplify dune files with dune 3.9's `build_if` (@talex5 ocaml-multicore/eio#582).

- Remove `Waiters` from `Eio_core` (@talex5 ocaml-multicore/eio#567).
  `Eio.Switch` no longer uses this so it can finally be removed.

- Use `Fmt.Dump.signal` to format signals (@talex5, @MisterDA ocaml-multicore/eio#543).

Documentation:

- Add some notes about thread-safety in the documentation (@talex5 ocaml-multicore/eio#568).
talex5 added a commit to talex5/opam-repository that referenced this pull request Jul 12, 2023
CHANGES:

New features / API changes:

- Extend `Eio.Condition` API (@talex5 ocaml-multicore/eio#563).
  - `loop_no_mutex` is a simpler and more efficient way to way for a condition.
  - `register_immediate` allows integration with other IO libraries.

- Expose `Eio.Stdenv.backend_id` (@bord-o ocaml-multicore/eio#560, reviewed by @talex5).
  Useful in tests to report which backend is being used.

- Remove deprecated features (@talex5 ocaml-multicore/eio#552, reviewed by @avsm).
  These were all already marked as deprecated in v0.10 and are now gone completely:
  - `Fiber.fork_sub`
  - `Eio_unix.{FD,Ipaddr,socketpair,getnameinfo}`
  - `Eio_linux.{FD,get_fd,get_fd_opt}`
  - `Eio_posix.Low_level.Fd`

- Allow calling `close` more than once (@talex5 ocaml-multicore/eio#547, requested by @anmonteiro, reviewed by @patricoferris, @avsm).

- Add `close` to socket type (@talex5 ocaml-multicore/eio#549).
  Simplifies the type signatures a bit by avoiding having to mention this everywhere.

Bug fixes:

- Fix handling of empty path strings (@talex5 ocaml-multicore/eio#569, reported by @SGrondin).
  Using "" instead of "." in some places resulted in an error.

- eio_posix: fix update to watched FDs on cancel (@talex5 ocaml-multicore/eio#574, reported and reviewed by @quernd).
  Cancelling the last watcher of an FD didn't remove it from the set passed to `poll`,
  which could result in constant wake-ups.

- eio_posix: fix `pread` at end-of-file (@talex5 ocaml-multicore/eio#581, reported by @SGrondin).
  It tried to return 0 instead of `End_of_file`, triggering an assertion.

- eio_posix: don't reap non-Eio child processes (@talex5 ocaml-multicore/eio#562).
  This allows spawning processes with e.g. the stdlib or Lwt
  (but see ocaml-multicore/lwt_eio#19 for Lwt support).

- Preserve backtraces across `Domain_manager.run` (@talex5 ocaml-multicore/eio#571).
  See ocaml/ocaml#12362.

- Correct the backend selection for Cygwin (@dra27 ocaml-multicore/eio#557).
  Use `eio_posix`, not `eio_windows` in this case.

Other changes:

- Simplify dune files with dune 3.9's `build_if` (@talex5 ocaml-multicore/eio#582).

- Remove `Waiters` from `Eio_core` (@talex5 ocaml-multicore/eio#567).
  `Eio.Switch` no longer uses this so it can finally be removed.

- Use `Fmt.Dump.signal` to format signals (@talex5, @MisterDA ocaml-multicore/eio#543).

Documentation:

- Add some notes about thread-safety in the documentation (@talex5 ocaml-multicore/eio#568).
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

New features / API changes:

- Extend `Eio.Condition` API (@talex5 ocaml-multicore/eio#563).
  - `loop_no_mutex` is a simpler and more efficient way to way for a condition.
  - `register_immediate` allows integration with other IO libraries.

- Expose `Eio.Stdenv.backend_id` (@bord-o ocaml-multicore/eio#560, reviewed by @talex5).
  Useful in tests to report which backend is being used.

- Remove deprecated features (@talex5 ocaml-multicore/eio#552, reviewed by @avsm).
  These were all already marked as deprecated in v0.10 and are now gone completely:
  - `Fiber.fork_sub`
  - `Eio_unix.{FD,Ipaddr,socketpair,getnameinfo}`
  - `Eio_linux.{FD,get_fd,get_fd_opt}`
  - `Eio_posix.Low_level.Fd`

- Allow calling `close` more than once (@talex5 ocaml-multicore/eio#547, requested by @anmonteiro, reviewed by @patricoferris, @avsm).

- Add `close` to socket type (@talex5 ocaml-multicore/eio#549).
  Simplifies the type signatures a bit by avoiding having to mention this everywhere.

Bug fixes:

- Fix handling of empty path strings (@talex5 ocaml-multicore/eio#569, reported by @SGrondin).
  Using "" instead of "." in some places resulted in an error.

- eio_posix: fix update to watched FDs on cancel (@talex5 ocaml-multicore/eio#574, reported and reviewed by @quernd).
  Cancelling the last watcher of an FD didn't remove it from the set passed to `poll`,
  which could result in constant wake-ups.

- eio_posix: fix `pread` at end-of-file (@talex5 ocaml-multicore/eio#581, reported by @SGrondin).
  It tried to return 0 instead of `End_of_file`, triggering an assertion.

- eio_posix: don't reap non-Eio child processes (@talex5 ocaml-multicore/eio#562).
  This allows spawning processes with e.g. the stdlib or Lwt
  (but see ocaml-multicore/lwt_eio#19 for Lwt support).

- Preserve backtraces across `Domain_manager.run` (@talex5 ocaml-multicore/eio#571).
  See ocaml/ocaml#12362.

- Correct the backend selection for Cygwin (@dra27 ocaml-multicore/eio#557).
  Use `eio_posix`, not `eio_windows` in this case.

Other changes:

- Simplify dune files with dune 3.9's `build_if` (@talex5 ocaml-multicore/eio#582).

- Remove `Waiters` from `Eio_core` (@talex5 ocaml-multicore/eio#567).
  `Eio.Switch` no longer uses this so it can finally be removed.

- Use `Fmt.Dump.signal` to format signals (@talex5, @MisterDA ocaml-multicore/eio#543).

Documentation:

- Add some notes about thread-safety in the documentation (@talex5 ocaml-multicore/eio#568).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problematic assert() in Eio.File.pread
3 participants