Skip to content

stdlib: remove Domain.at_each_spawn#11595

Merged
Octachron merged 5 commits intoocaml:trunkfrom
Octachron:remove_at_each_spawn
Oct 6, 2022
Merged

stdlib: remove Domain.at_each_spawn#11595
Octachron merged 5 commits intoocaml:trunkfrom
Octachron:remove_at_each_spawn

Conversation

@Octachron
Copy link
Copy Markdown
Member

This PR proposes to remove the function Domain.at_each_spawn.

One major issue with Domain.at_each_spawn is that it is impossible to predict in which context the registered function will end up being called.

A good illustration of the issue at hand is the use of Domain.at_each_spawn in the threads library in order to initialize the thread machinery. This approach breaks down if the order of the initialization is incorrect as described in #11434. Consider for instance, a module Early_hook registering a function with Domain.at_each_spawn

(* early_hook.ml *)
let f = ref ignore
let () = Domain.at_each_spawn (fun () -> !f ())

and the following main function:

(* main.ml *)
let print_self () =
  Printf.printf "self=%d\n" (Thread.id (Thread.self ()))
let () =
  Early_hook.f := print_self;
  for i = 0 to 50 do
    Domain.join (Domain.spawn ignore)
  done

With this setup, any program compiled with early_hook linked before the systhreads library crashes because the function registered by early_hook calls Thread.self before the thread machinery initialization.

The first commit in this PR fixes this issue by adding a new C hook caml_domain_initialize_hook for the systhreads library which is run during domain initialization just before the registration with the stop-the-world mechanism. (I am not sure of the right location for the hook during initialization). The second commit adds the example described above as an ephemeral test.

The two last commits removes the use of Domain.at_each_spawn in Format by moving the registration of the Domain.at_exit functions to the initialization of the by-domain stdout and stder formatter.

Finally, the last commit completely remove Domain.at_each_spawn function since the function seems more harmful than helpful in its current state. It is thus probably better in term of forward compatibility to remove it.

Close #11434

@dra27 dra27 added this to the 5.0 milestone Oct 3, 2022
Copy link
Copy Markdown
Contributor

@gadmm gadmm left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me apart from the location where the hook is called, but I had a fairly quick look. Would @Engil or @gasche be willing to give a second opinion?

runtime/domain.c Outdated

caml_reset_young_limit(domain_state);

caml_domain_initialize_hook();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is called while holding the all_domains lock. It is better to wait that the lock is released. I would call caml_domain_initialize_hook right before the call to caml_callback inside domain_thread_func.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That seems better indeed.

Copy link
Copy Markdown
Contributor

@kayceesrk kayceesrk left a comment

Choose a reason for hiding this comment

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

The change looks good to me. Removing Domain.at_each_spawn is a good move.

@Octachron Octachron force-pushed the remove_at_each_spawn branch from 8c7309c to edb3955 Compare October 5, 2022 13:06
@Octachron
Copy link
Copy Markdown
Member Author

I have cleaned the history and I am planning to merge tomorrow in order to move forward the release cycle.

@Octachron Octachron merged commit 021619f into ocaml:trunk Oct 6, 2022
Octachron added a commit that referenced this pull request Oct 6, 2022
stdlib: remove `Domain.at_each_spawn`
(cherry picked from commit 021619f)
@Octachron Octachron deleted the remove_at_each_spawn branch October 7, 2022 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NULL pointer dereferencing and use-after-free in systhreads due to delayed init

5 participants