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

async_io: adapt self-pipe trick to Windows #8044

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Jun 25, 2023

The Dune RPC server uses the "self-pipe trick" to interrupt Unix.select. Unfortunately, on Windows, pipes cannot be passed to the native select system call, which means that Unix.select falls back to a complicated emulation using threads to poll for data on the pipe. This, plus the fact that pipes are always blocking, seem to be causing some deadlock somewhere which makes the RPC server hang under certain circumstances.

This PR proposes an alternative form of the self-pipe trick using a self-connected UDP socket. The advantages is that no pipes are involved, so only the native select call is used by Unix.select, completely bypassing all the complexity of the emulation and the associated blocking pipes.

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

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

Awesome work. I imagine it's worth reporting that select on windows pipes fails for this case?

Needs a CHANGES entry.

@nojb
Copy link
Collaborator Author

nojb commented Jun 26, 2023

Awesome work. I imagine it's worth reporting that select on windows pipes fails for this case?

My understanding is that it is not that it doesn't work, but rather that the semantics of pipes on Windows do not match exactly those of Linux and the emulation in Unix.select does not help things, in the sense that there may be a way to make the existing approach work (probably involving further synchronization), but it becomes more and more complex and it becomes hard to know what the right solution is.

Needs a CHANGES entry.

Thanks, will add. I also want to wait to see if by chance this PR also fixes #8043.

@emillon emillon mentioned this pull request Jun 23, 2023
16 tasks
nojb and others added 2 commits June 26, 2023 17:43
Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb force-pushed the async_io_self_pipe_win32 branch from e477b72 to 58fb66c Compare June 26, 2023 15:43
@nojb nojb merged commit 71024ef into ocaml:main Jun 26, 2023
20 of 21 checks passed
@nojb nojb deleted the async_io_self_pipe_win32 branch June 26, 2023 16:44
@nojb
Copy link
Collaborator Author

nojb commented Jun 26, 2023

@emillon if there is a bug fix release 3.8.3 this should be in it.

@emillon emillon mentioned this pull request Jun 27, 2023
6 tasks
emillon pushed a commit to emillon/dune that referenced this pull request Jun 27, 2023
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
emillon pushed a commit to emillon/dune that referenced this pull request Jun 27, 2023
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
emillon added a commit that referenced this pull request Jun 27, 2023
* async_io: adapt self-pipe trick to Windows (#8044)

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

* Update CHANGES.md

Signed-off-by: Etienne Millon <etienne.millon@gmail.com>

---------

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Etienne Millon <etienne.millon@gmail.com>
Co-authored-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
emillon added a commit to emillon/opam-repository that referenced this pull request Jun 27, 2023
CHANGES:

- Fix deadlock on Windows (ocaml/dune#8044, @nojb)

- When using `sendfile` to copy files on Linux, fall back to the portable
  version if it fails at runtime for some reason (NFS, etc).
  (ocaml/dune#8049, fixes ocaml/dune#8041, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants