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

Make sure a function registered with `at_exit` is executed only once #1790

Merged
merged 2 commits into from May 23, 2018

Conversation

Projects
None yet
6 participants
@xavierleroy
Copy link
Contributor

xavierleroy commented May 21, 2018

As shown in MPR#7253 and MPR#7796, there are several cases where do_at_exit is called several times, causing functions registered with at_exit to be called several times. Also, an at_exit function that raises could prevent other at_exit functions from being run.

This GPR makes sure that each function registered with at_exit is run at most once. The idea and implementation is due to @nojb in #1783 (comment).

Implementation notes:

  • The test-and-set on a bool ref is atomic with the two thread libraries of OCaml.
  • Consequently, if two threads race to call exit, each at_exit function will be called exactly once, but possibly not in the normal order.
  • Lazy evaluation (as suggested by @xclerc) would work too, except that the Lazy module is not available in Stdlib.
Make sure a function registered with "at_exit" is executed only once
Fixes: MPR#7253, MPR#7796.

As shown in the PRs above there are several cases where do_at_exit
is called several times, causing functions registered with at_exit
to be called several times.  Also, an at_exit function that raises
could prevent other at_exit functions from being run.

This commit doesn't try to prevent multiple calls to do_at_exit,
but makes sure that each function registered with at_exit is run
at most once.  The idea is due to Nicolás Ojeda Bär.
@xclerc

This comment has been minimized.

Copy link
Contributor

xclerc commented May 21, 2018

(For the record, I mentioned lazyness for use on client code
in the case the API required the registered functions to be
idempotent.)

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 21, 2018

Well, lazyness could have been used in the implementation of at_exit as well, with possibly stronger atomicity guarantees in Multicore OCaml, but dependencies dictated otherwise.

@xavierleroy xavierleroy added the bug label May 21, 2018

@xavierleroy xavierleroy added this to the 4.07 milestone May 21, 2018

@murmour

This comment has been minimized.

Copy link
Contributor

murmour commented May 21, 2018

This seems to be a robust solution to the "double execution" problem. I see no issues with it.

Since you're fixing at_exit, then perhaps it's a good moment to get rid of a different kind of a race condition -- the one in at_exit itself, which happens because of VM thread preemption during the allocation of a closure?

One possible approach:

let at_exit f =
  let g = ref (fun () -> ()) in
  let f_already_ran = ref false in
  let f () =
    if not !f_already_ran then (f_already_ran := true; f ());
    !g ()
  in
  g := !exit_function;
  exit_function := f

This is the silliest piece of code I wrote today, but it illustrates the idea.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

dbuenzli commented May 22, 2018

Also, an at_exit function that raises could prevent other at_exit functions from being run.

This problem is not fixed by this PR right ?

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 22, 2018

@murmour, rour code can also be made more readable:

let once f =
  let has_run = ref false in
  fun () ->
    if !has_run then ()
    else (has_run := true; f ())

let add_before f gref =
  (* using (gref := fun () -> f (); !gref ()) would introduce
     an infinite loop; instead we define old_g to be
     the value of !gref before the assignment *)
  let old_g = ref !gref in
  let seq () = f (); !old_g () in
  (* gref may be mutated concurrently during the allocation of seq,
     so we made old_g a reference and update it afterwards. *)
  old_g := !gref;
  gref := seq

let at_exit f =
  add_before (once f) exit_function
@shindere

This comment has been minimized.

Copy link
Contributor

shindere commented May 22, 2018

@murmour

This comment has been minimized.

Copy link
Contributor

murmour commented May 22, 2018

@gasche

your code can also be made more readable:

Yes, totally. The change to avoid a race condition is unobvious. On the other hand, I find the original code (of this PR) very readable, with no need for improvements.

Perhaps it'd suffice just to add a couple of comments?

let at_exit f =
  let g = ref (fun () -> ()) in (* a placeholder for !exit_function *)
  let f_ran = ref false in
  let f () =
    if not !f_ran then (f_ran := true; f ());
    !g ()
  in
  (* The following is thread-safe
     (indirection in g was added to avoid a race condition) *)
  g := !exit_function;
  exit_function := f
@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 22, 2018

@dbuenzli asks:

This problem [an at_exit function that raises could prevent other at_exit functions from being run] is not fixed by this PR right ?

Amusingly, the example you gave in MPR#7253 actually works, and is reproduced by the test I added. An exception is raised, then the Printexc code that reports this uncaught exception calls do_at_exit again. The at_exit functions already executed are skipped, including the one that raises the exception, and the remaining at_exit functions are run.

If you have several at_exit functions that raise, some other at_exit functions may still be skipped.

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 23, 2018

I intend to merge the code in its present state very soon, based on @murmour's positive review. Holler if you're not happy.

As to making at_exit atomic, the implementation above is probably correct with both thread libraries, but could we do this in a different GPR?

@murmour

This comment has been minimized.

Copy link
Contributor

murmour commented May 23, 2018

As to making at_exit atomic, the implementation above is probably correct with both thread libraries, but could we do this in a different GPR?

It won't hurt to be more atomic, yes.

@xavierleroy xavierleroy merged commit ab1ca8e into ocaml:trunk May 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@xavierleroy xavierleroy deleted the xavierleroy:at-exit-once branch May 23, 2018

xavierleroy added a commit that referenced this pull request May 23, 2018

Make sure a function registered with `at_exit` is executed only once (#…
…1790)

Fixes: MPR#7253 (in large part), MPR#7796 (in full).

As shown in the PRs above there are several cases where do_at_exit
is called several times, causing functions registered with at_exit
to be called several times.  Also, an at_exit function that raises
could prevent other at_exit functions from being run.

This commit doesn't try to prevent multiple calls to do_at_exit,
but makes sure that each function registered with at_exit is run
at most once.  The idea is due to Nicolás Ojeda Bär.
@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 23, 2018

Cherry-pick to 4.07: 6ce1e6d

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 23, 2018

Even better: this pull request fixes MPR#7178 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.