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.

Sign up for GitHub

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

Jump to bottom

Eio_posix: fix update to watched FDs on cancel #574

Merged
merged 2 commits into from
Jul 8, 2023
Merged

Eio_posix: fix update to watched FDs on cancel #574

merged 2 commits into from
Jul 8, 2023

Conversation

talex5
Copy link
Collaborator

@talex5 talex5 commented Jul 7, 2023

If cancelling removes the last waiter, we need to update the set of FDs being watched.

Fixes #572.

Reported by @quernd

@talex5
Eio_posix: fix update to watched FDs on cancel
cc19aa1 
Reported by Daniel Quernheim.
@quernd quernd mentioned this pull request Jul 7, 2023
Closed
@anmonteiro
Copy link
Contributor

This also fixes an issue I was seeing where EIO reported successful writes on closed file descriptors.

@talex5
Workaround CI solver limitation
fc68c00 
This temporarily reverts ocaml-multicore#542 until the CI supports it.
@talex5 talex5 merged commit 57ace76 into ocaml-multicore:main Jul 8, 2023
@talex5 talex5 deleted the fix-invalid-fd branch July 8, 2023 10:17
@talex5
Copy link
Collaborator Author

talex5 commented Jul 8, 2023

This also fixes an issue I was seeing where EIO reported successful writes on closed file descriptors.

Closing a file prevents new write operations from starting, but existing ones may be allowed to complete. I don't see why this PR should affect that; the bug will cause an extra wake-up but the write should just get EAGAIN and go back to sleep immediately.

@quernd
Copy link

quernd commented Jul 8, 2023

I was a bit nervous about the assert but it appears the logic is sound. I ran some long-running load tests and the program did not crash or exhibit any pathological behaviour.
Thanks for the quick response 👍

talex5 added a commit to talex5/opam-repository that referenced this pull request Jul 12, 2023
@talex5
[new release] eio (5 packages) (0.11)
cb0efd6 
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
@talex5
[new release] eio (5 packages) (0.11)
3908305 
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 talex5 mentioned this pull request Jul 12, 2023
Merged
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@talex5 @nberth
[new release] eio (5 packages) (0.11)
ddf615d 
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
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

POSIX backend keeps polling fd after close and ignores POLLNVAL
3 participants
@talex5 @anmonteiro @quernd