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
Comments
Comment author: @dbuenzli Forgot to add, this is not a toplevel problem. The problem also occurs on executables. |
Comment author: @dbuenzli The fix doesn't seem too hard: the call to the callbacks in the chaining should be protected by handler here: Line 515 in 393f068
The double call is likely to come from the call to Pervasives.do_at_exit by printexc.ml on unhandled exception: Line 260 in 393f068
So if at_exit chaining properly catches exceptions and prints them I guess the double call should disappear. |
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. |
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:
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" ? |
Comment author: @mshinwell @dbuenzli: Has there been any further progress on this? #685 appears to have been closed without being merged. |
Comment author: @dbuenzli No. I closed it because it was not a good solution. |
Comment author: @mshinwell Do you want to keep this Mantis issue open or not? |
Comment author: @dbuenzli Well it is an issue no ? |
Comment author: @xavierleroy Alternate fix proposed here: #1790 |
Comment author: @xavierleroy Merged #1790. It fixes
I'll let @dbuenzli decide whether to keep this Mantis issue open. |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
I still think it would be nice to have the isolation guarantee that if an at exit handler raises it doesn't prevent others from running. |
This sounds like something that @gadmm has a lot of opinions about. We discussed related questions in the setting of statmemprof, but it is not at all easy (to me) to decide what to do. In favor of dropping the exception and continuing with other callbacks:
Against:
Personally this sort of questions makes me a bit wary of using |
I agree with @dbuenzli. @lthls you had opinions about this at #2118.
Review of For asynchronous exceptions mentioned by @gasche, it will be possible to even mask them while the handlers are running. In my implementation of As part of evidence-based programming language design, it would be good to audit the uses of A bit off-topic: do not take this message as evidence that the stale bot is a good idea (although I thank @gasche for pinging me); I am a bit busy and felt pressured by the bot. |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
Not stale, but the discussion has continued at #11221 as far as I can tell. |
The two repro cases that @dbuenzli used to open this PR are now working correctly. I understand from tying to re-read this discussion that some issues still exist when an exception is raised by one of the |
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):
Steps to reproduce
The text was updated successfully, but these errors were encountered: