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

fix: disable background threads where we use fork #8100

Merged

Conversation

rgrinberg
Copy link
Member

Signed-off-by: Rudi Grinberg me@rgrinberg.com

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

<!-- ps-id: 57f4c29c-44d5-4089-a81b-01ae28e5377a -->
@rgrinberg rgrinberg force-pushed the ps/rr/fix__disable_background_threads_where_we_use_fork branch from 61be40b to 57f9a7c Compare July 4, 2023 09:32
@rgrinberg
Copy link
Member Author

@anmonteiro just to make sure, did you double check this fix worked?

@emillon
Copy link
Collaborator

emillon commented Jul 5, 2023

is the hang on macos CI related to the fix?

@rgrinberg
Copy link
Member Author

Indeed it's likely related. If the fix is indeed what I think it is, only #8090 is a complete fix.

@anmonteiro
Copy link
Collaborator

@anmonteiro just to make sure, did you double check this fix worked?

Yes.

@rgrinberg
Copy link
Member Author

@emillon this is good enough for 3.9.1

@rgrinberg rgrinberg merged commit 4018879 into main Jul 5, 2023
@rgrinberg rgrinberg deleted the ps/rr/fix__disable_background_threads_where_we_use_fork branch July 5, 2023 15:55
@emillon
Copy link
Collaborator

emillon commented Jul 5, 2023

I'm a bit concerned that this still hangs on macos. Do you understand why that's the case? Do you have an idea if this makes that less likely than on 3.9.0? If we reverted the original feature, would it hang sometimes as well?

@rgrinberg
Copy link
Member Author

I assume it's just because it's not safe to fork in a multi threaded application in OCaml. We still have other uses of threads, and it's not really possible to get rid of them all.

@emillon
Copy link
Collaborator

emillon commented Jul 5, 2023

I understand that #8090 will fix that more or less permanently, but it's probably too risky for a backport. So I'm wondering if there's a good way to mitigate what it looks to be a regression (due to the fact we're using more threads than before I suppose)

@rgrinberg
Copy link
Member Author

Thinking a little more. We could mitigate the issue a little more if we:

  1. Disable the threaded console on platforms where we use fork.
  2. Lazy initialize the Async_io thread. It's only needed in watch mode, so we can disable it for normal builds.

Do you want to give these a try?

@emillon
Copy link
Collaborator

emillon commented Jul 5, 2023

Sure! I suppose that the first one is about doing the same as this PR, but for threaded_console below?

@rgrinberg
Copy link
Member Author

Yes, basically.

@emillon
Copy link
Collaborator

emillon commented Jul 5, 2023

What about the second one? I see Async_io.with_io in the scheduler but I know nothing about that part of Dune, so tips appreciated :)

@rgrinberg
Copy link
Member Author

Np. At the moment, the various functions in Async_io dispatch the work to the select thread (e.g. ready, close, etc.). Instead of starting the thread eagerly in with_io, we can wait until one of those functions is called before initializing the thread.

emillon added a commit to emillon/dune that referenced this pull request Jul 6, 2023
This is similar to ocaml#8100 for the threaded console. The reason we're
doing that is to avoid threads when fork is involved.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/dune that referenced this pull request Jul 6, 2023
This is similar to ocaml#8100 for the threaded console. The reason we're
doing that is to avoid threads when fork is involved.

Signed-off-by: Etienne Millon <me@emillon.org>
@emillon
Copy link
Collaborator

emillon commented Jul 6, 2023

I've done that in #8121 and #8122.

@emillon emillon mentioned this pull request Jul 6, 2023
9 tasks
emillon added a commit that referenced this pull request Jul 6, 2023
This is similar to #8100 for the threaded console. The reason we're
doing that is to avoid threads when fork is involved.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon pushed a commit to emillon/dune that referenced this pull request Jul 6, 2023
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
emillon added a commit to emillon/dune that referenced this pull request Jul 6, 2023
This is similar to ocaml#8100 for the threaded console. The reason we're
doing that is to avoid threads when fork is involved.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this pull request Jul 6, 2023
Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
Co-authored-by: Rudi Grinberg <me@rgrinberg.com>
emillon added a commit to emillon/dune that referenced this pull request Jul 6, 2023
This is similar to ocaml#8100 for the threaded console. The reason we're
doing that is to avoid threads when fork is involved.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit that referenced this pull request Jul 6, 2023
This is similar to #8100 for the threaded console. The reason we're
doing that is to avoid threads when fork is involved.

Signed-off-by: Etienne Millon <me@emillon.org>
emillon added a commit to emillon/opam-repository that referenced this pull request Jul 6, 2023
CHANGES:

- Disable background operations and threaded console on MacOS and other Unixes
  where we rely on fork. (ocaml/dune#8100, ocaml/dune#8121, fixes ocaml/dune#8083, @rgrinberg, @emillon)

- Initialize async IO thread lazily. (ocaml/dune#8122, @emillon)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

- Disable background operations and threaded console on MacOS and other Unixes
  where we rely on fork. (ocaml/dune#8100, ocaml/dune#8121, fixes ocaml/dune#8083, @rgrinberg, @emillon)

- Initialize async IO thread lazily. (ocaml/dune#8122, @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.

3 participants