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

at_exit functions get called twice if a callback raises and prevents earlier handlers to execute. #7253

Open
vicuna opened this Issue May 10, 2016 · 11 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link
Collaborator

vicuna commented May 10, 2016

Original bug ID: 7253
Reporter: @dbuenzli
Status: acknowledged (set by @damiendoligez on 2016-05-10T13:34:08Z)
Resolution: open
Priority: normal
Severity: minor
Version: 4.03.0
Target version: later
Category: standard library
Tags: junior_job
Related to: #7796
Monitored by: dexterph @hcarty @dbuenzli

Bug description

The behaviour in case of an [at_exit] callback raising is a little bit suboptimal (slightly related to #7178):

  1. A raising callback prevents callbacks registered earlier to be executed.
  2. A raising callback and all the callbacks that come after get called twice.

Steps to reproduce

(* 1. prevents callbacks from running *)

let () = at_exit (fun () -> print_endline "Once";);;

let () = at_exit (fun () -> print_endline "Twice"; raise Exit);;

^D

Twice
Twice
Fatal error: exception Pervasives.Exit
...

(* 2. Double calls *)

let () = at_exit (fun () -> print_endline "Twice"; raise Exit);;

let () = at_exit (fun () -> print_endline "Once";);;

^D

Once
Twice
Once
Twice

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 10, 2016

Comment author: @dbuenzli

Forgot to add, this is not a toplevel problem. The problem also occurs on executables.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 10, 2016

Comment author: @dbuenzli

The fix doesn't seem too hard: the call to the callbacks in the chaining should be protected by handler here:

exit_function := (fun () -> f(); g())

The double call is likely to come from the call to Pervasives.do_at_exit by printexc.ml on unhandled exception:

(try Pervasives.do_at_exit () with _ -> ());

So if at_exit chaining properly catches exceptions and prints them I guess the double call should disappear.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 1, 2016

Comment author: dexterph

The double calls do disappear, however there is no clear way to print an exception from within at_exit in Pervasives. I think a more complicated change would need to happen.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 11, 2016

Comment author: @dbuenzli

So I have a fix for this which works so that each uncaught exception in at_exit functions get reported. However this slightly changes the semantics of the callback set by Printexc.set_uncaught_exception_handler function (introduced in 4.02) since it is supposed there that:

"Note that when fn is called all the functions registered with at_exit have already been called. Because of this you must make sure any output channel fn writes on is flushed."

With this fix, the "have already been called" becomes "may already have been called", since I try to call the at_exit handlers as much as possible until one raises in which case I record the raise, handle the uncaught exception and then handle the at_exit function uncaught exception using the same mechanism.

This could be one option. However I also have the feeling that this way of operating makes it less easy to understand what is happening, for example:

let () = at_exit (fun () -> print_endline "bla");;

let () = at_exit (fun () -> raise Exit);;

let () = at_exit (fun () -> print_endline "bli");;

let () = at_exit (fun () -> failwith "HEY");;

^D

bli
Fatal error: exception Failure("HEY")
Raised at file "pervasives.ml", line 32, characters 22-33
Called from file "pervasives.ml", line 519, characters 35-39
Called from file "pervasives.ml", line 527, characters 4-17
Called from file "toplevel/toploop.ml", line 547, characters 21-27
Called from file "toplevel/topstart.ml", line 16, characters 8-22
bla
Fatal error: exception Pervasives.Exit
Raised at file "//toplevel//", line 1, characters 34-38
Called from file "pervasives.ml", line 519, characters 35-39
Called from file "printexc.ml", line 263, characters 12-36

So here bli gets printed first because HEY gets uncaught, we try to call as much as possible the at_exit handlers, which leads bli to get called, then Exit is uncaught, so we print HEY and start the uncaught loop for Exit, which leads bla to get called and finally Exit to be printed.

Now it would be as easy and possible to execute and print this in the natural order. So my question is what was the rationale behind calling at_exit functions before the handler ? Would it be a problem to change the semantics so that:

"Note that when fn is called all functions that are registered with at_exit that have not been called yet will be called after the call to the handler" ?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jul 12, 2016

Comment author: @dbuenzli

A fix is proposed in #685. W.r.t. to my previous message it is the version that tries to retain a natural call order w.r.t. to effects.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 8, 2016

Comment author: @mshinwell

@dbuenzli: Has there been any further progress on this? #685 appears to have been closed without being merged.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 8, 2016

Comment author: @dbuenzli

No. I closed it because it was not a good solution.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 8, 2016

Comment author: @mshinwell

Do you want to keep this Mantis issue open or not?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Dec 8, 2016

Comment author: @dbuenzli

Well it is an issue no ?

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 21, 2018

Comment author: @xavierleroy

Alternate fix proposed here: #1790

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 23, 2018

Comment author: @xavierleroy

Merged #1790. It fixes

  • issue 2 (A raising callback and all the callbacks that come after get called twice)
  • some cases of issue 1 (if there is only one raising callback, the callbacks registered earlier will be executed; if one of those raises again, the callbacks registered earlier than that one will not run).

I'll let @dbuenzli decide whether to keep this Mantis issue open.

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.