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

protect: treat as an error the case where two exceptions are being raised #2118

Merged
merged 1 commit into from Feb 3, 2019

Conversation

Projects
None yet
@gadmm
Copy link
Contributor

commented Oct 25, 2018

Thanks to @mseri and #1855, we now have protect in Stdlib analogous to unwind-protect from Lisp. I do not know whether the following behaviour has been discussed already: what happens when both the protected function and the finally raise. This PR aims to treat this case as an error.

To me, there does not seem to be any good way to behave in this case, and it is similar to C++/Rust where exceptions/panics in destructors may cause the program to abort. In contrast, the current version proposes that the exception from the finally clause shadows that from the function. This choice does not seem obvious to me, and Jane Street's Base.Exn.protect also behaves differently: if both functions raise, a new exception Finally is raised.

The behaviour from Base.Exn.protect seems preferable to me: my rationale is that this situation is an actual error not meant to be caught as part of the application logic, yet one will still try to clean-up other resources. A consequence is to discourage programmers from raising exceptions in finalizers.

Given the precedent, this PR proposes to simply copy this behaviour from Base.Exn.protect and introduce a new exception Finally of exn * exn in Stdlib. (Note in particular that this PR copies the documentation of this exception from Jane Street.)

There is a choice, in the case where Finally is being raised, whether to restore the previous backtrace or not. For now this patch does not restore it, but it is open to discussion.

@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

This is targetting 4.08.0, because the behaviour cannot be changed after that.

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

(cc @bobot @nojb @xclerc, who were involved in previous discussions on this function)

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

I think a very good case can be made for @gadmm's proposal but I'm not convinced by his rationale. So I will add my own (maybe that's what he wanted to say).

The current behaviour and doc string of protect very weakly implies that raising an unexpected exception from a finalizer is a programming error. However case arising, say for example an unhandled Failure, the exception might be caught by a legitimate surrounding exception handler which results in a complete obfuscation of the programming error. Using the Finally exception allows to ensure the programming error is revealed -- under the reasonable assumption that like Invalid_argument, programmers never try to catch that exception except at the toplevel of their program.

One objection that I have is to add a new exception in Pervasives; but that shouldn't be a problem if we simply move all of this to a potential Fun module, which I think should be done regardless of what happen with the latter PR.

@alainfrisch alainfrisch added the stdlib label Oct 26, 2018

@bobot

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

There is a choice, in the case where Finally is being raised, whether to restore the previous backtrace or not.

Both backtrace could be added to the Finally exception. In any case we should ensure that this exception is by default printed in a nice way when it reaches the toplevel.

Using the Finally exception allows to ensure the programming error is revealed

We could go further and always wrap the exception coming from the finalizer.

@lthls

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

I agree with @bobot here: you could define your exception as:

exception Finally of { worker : exn option; finalizer : exn; }

and wrap both calls to finally in a try ... with (or match ... with exception) structure.

This way, you can document that the finalizer is known to have run successfully except in the case where a Finally exception is thrown (in which case, it may have been raised either by protect or by the worker itself).

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2018

We could go further and always wrap the exception coming from the finalizer.

I agree. If we consider that the finalizer should not "usually" raise exceptions (because there is no nice way to deal with them when both the finalizer and the payload raise), we should try to detect the situation as early as possible, including in the normal case where the payload itself does not raise.

@dbuenzli dbuenzli referenced this pull request Nov 1, 2018

Merged

Stdlib: add Fun module. #2129

@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

Given the discussion (especially the desire to treat exceptions coming out of the finalizer as an error in all cases), I gave further thought about what would be the right thing to do.

OCaml currently supports asynchronous exceptions, and I am afraid that protect as implemented in the standard library has the same issues as asynchronous exceptions raise in multicore. Two relevant discussions of this topic are:

To simplify let us be strict and prevent exceptions from being raised from finally. (The same issues arise if one collects the two exceptions in a serious exception Finally of exn * exn, because this may convert a catchable exception into a serious one.)

let run_no_err f x = try f x with _ -> exit 1

let protect ~finally work =
  match work () with
  | result -> let () = run_no_err finally () in result
  | exception e -> let () = run_no_err finally () in raise e

This is wrong! Indeed, finally () can raise asynchronous exceptions and therefore crash the program. This includes the Sys.Break exception, and any exception thrown from the user's signal handlers. (They are the only true asynchronous exceptions; exceptions coming out of finalizers via the GC are programming errors, and Out_of_memory is also problematic but something else.)

Dolan et al. explain that the solution is to mask and unmask asynchronous exceptions, like is done by bracket in Haskell. But assume that we have a proper way to mask and unmask asynchronous exceptions (i.e. define critical sections that delays the exception).

val is_masked : unit -> bool
val set_mask : unit -> unit
val clear_mask : unit -> unit

A naive way is to do as follows:

let run_no_err_no_async f x =
  let was_masked = is_masked () in
  if not was_masked then set_mask ();
  let result = try f x with _ -> exit 1 in
  if not was_masked then clear_mask ();
  result

let protect_with_mask ~finally work =
  match work () with
  | result -> let () = run_no_err_no_async finally () in result
  | exception e -> let () = run_no_err_no_async finally () in raise e

But this is not enough. Indeed, asynchronous exceptions also need to be masked between the creation of the resource and the call to protect.

let with_file_wrong fname ~f =
  let fd = open_in fname in
  protect_with_mask ~finally:(fun () -> close_in_noerr fd) (fun () -> f fd)

Here the resource leaks if an async exception occurs between open_in and protect_with_mask.

Provided we have access to masking primitives above, the correct thing to do in the presence of asynchronous exceptions is to implement protect with an acquisition function, like the following:

module Async_exn : sig
  val mask : (bool -> 'a) -> 'a
  val unmask : was_masked:bool -> (unit -> 'a) -> 'a
end = struct
  let mask f =
    if is_masked () then f true
    else (
      let work () = f false in
      (* no allocations => atomic for async exns *)
      set_mask ();
      protect ~finally:clear_mask work
    )

  let unmask ~was_masked f =
    if was_masked then f ()
    else (
      (* no allocations => atomic for async exns *)
      clear_mask ();
      protect ~finally:set_mask f
    )
end

let new_protect ~acquire ~finally work =
  Async_exn.mask @@ fun was_masked ->
  let res = acquire () in
  protect ~finally:(fun () -> finally res)
    (fun () -> Async_exn.unmask ~was_masked @@ fun () -> work res)

val new_protect : acquire:(unit -> 'a) -> finally:('a -> unit) -> ('a -> 'b) -> 'b

For the purpose of implementing the with_ idiom, this is functionally equivalent to the previous version:

let with_file fname ~f =
  new_protect ~acquire:(fun () -> open_in fname) ~finally:close_in_noerr f

To summarise, the current version of protect can shadow serious exceptions with non-serious ones; even at no fault of the finally clause due to asynchronous exceptions. The version currently in the PR is incorrect due to asynchronous exceptions. The correct version is inspired by Haskell's bracket, masks asynchronous exceptions, and takes an additional acquisition function.

There is also the consideration of whether asynchronous exceptions are going to be removed in the long term or not, and there is a discussion about this at ocaml-multicore/ocaml-multicore#100. But for the moment they are part of the language, and it is easier to have a correct protect now and remove async exceptions later, than the contrary.

Functions is_masked, set_mask, clear_mask are missing for now. @stedolan, you told me a while ago that you have plans to implement masking in multicore, and I saw that this is something you might need even in the event where asynchronous exceptions were replaced by asynchronous effects. How applicable would that be for the current issue?

Depending on the availability of masking, I propose that I close this PR and open a bug report.

Inviting @kayceesrk and @lpw25, who are also involved in the discussions about asynchronous exceptions.

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

One somewhat pressing question is whether we want protect in the current form included in 4.08 (the next OCaml release), it is currently only in the trunk/development version. Can you see a future-proof change to the interface, even if masking is unresolved? Or do we have reasonable confidence that there is no good solution right now? Should we delay the release of protect, or can we release in the current state (or with a modified proposal) and provide a second, better function later?

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

But for the moment they are part of the language, and it is easier to have a correct protect now and remove async exceptions later, than the contrary.

I think this is the key point. I'm a firm advocate of removing asynchronous exceptions from OCaml as part of multicore OCaml -- see the discussion that @gadmm linked -- but its still better to make something which is correct in the current world.

Can you see a future-proof change to the interface, even if masking is unresolved?

Isn't this such an interface:

val new_protect : acquire:(unit -> 'a) -> finally:('a -> unit) -> ('a -> 'b) -> 'b
@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

Can you see a future-proof change to the interface, even if masking is unresolved? Or do we have reasonable confidence that there is no good solution right now? Should we delay the release of protect, or can we release in the current state (or with a modified proposal) and provide a second, better function later?

I am not sure what you are asking. If OCaml supports asynchronous exceptions, and if protect is introduced in the standard library to fit some purpose (I imagine resource management), then the closest known correct version uses masking and an acquire function.

Currently, protect (in trunk or in the PR) do what their documentation say, and are probably useful to many OCaml developers, but do not have a precise and general purpose (e.g. something that library writers can rely on to write correct with_… functions for resource management).

I imagine an argument could go along the lines of “there is no sound implementation of unwind-protect in OCaml with asynchronous exceptions anyway, so we may as well split OCaml into two dialects, one with resource management and one with asynchronous exceptions (probably already the case in practice), in the hope that one day we could reunite the two magically by adding masking”. Is this what you imply? If so, the answers are 1) the best future-proof API is one that will need to be changed later because it will force developers to review their usage of protect (I am not saying it is desirable, only better), 2) yes, 3) not me to decide :)

I'll still wait a bit before closing the PR and opening a bug report in case I get an answer about the difficulty of implementing masking.

@stedolan

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

Can you see a future-proof change to the interface, even if masking is unresolved?

Isn't this such an interface:

val new_protect : acquire:(unit -> 'a) -> finally:('a -> unit) -> ('a -> 'b) -> 'b

I agree, this is just the interface that's needed to make protect future-proof. The important point is that the implementation of new_protect controls the context in which the resource is acquired, so a future implementation can use masking to eliminate the race between the resouce being acquired and the finalizer being enabled. (Incidentally, this interface is roughly the same as Haskell's bracket)

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

So shouldn't we change protect today to use its interface, before it is first released to the public? If you agree, I would suggest to fast-track that particular change. (I would also like to see the present suggestion and its discussion converge rather quickly, before we start releasing 4.08).

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

@gasche One thing that needs to happen for sure is that protect should be moved to the new Fun module rather than added to Pervasives before 4.08 is released.

Also I don't think it's a good idea to provide a half-broken implementation of protect so if the discussion doesn't converge I think we should simply drop protect as it currently exists in trunk.

If @gadmm doesn't have the time I can make a PR along the new_protect line as soon as possible.

@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

The reasoning would be that giving the users the good interface with the wrong implementation would be better than the statu quo?

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

The reasoning would be that giving the users the good interface with the wrong implementation would be better than the statu quo?

I'd say that in reality the problem is not as acute as the discussion might make it sound.

I see the fact that end-users raising in signal handlers leads to user definable asynchronous exception a bug rather than a feature of the runtime system since none of the existing ressource handling ocaml code out there is able to deal this with this correctly (I also suspect it would break things like lwt).

So if we leave this use case out we are left with two asynchronous exception: Break and Out_of_memory, the first one you'll get it only in the toplevel interaction where a resource leaking here and there may not be catastrophic (it would of course be good if we could do the correct thing), the second one you are unlikely to be able to do anything meaningful but abort the program.

So by providing the new_protect function we solve maybe now 99% of the uses case and make it future proof so that 100% of the cases can be handled whenever the right tools will be there.

@gasche

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

Yes, my idea was to keep the function, with a future-proof interface so that it can be fixed later. I'm also okay with removing the function, but I find it a harder sell. I'm not sure why we keep talking about a new_protect function; I certainly feel that we shouldn't, at any point, have two protect functions available, a "broken" one and a "fixed" one, and ask users to choose carefully.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

@gasche of course I'm calling it new_protect just to make the distinction about which signature we are talking that should be the only protect function.

@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

One thing that needs to happen for sure is that protect should be moved to the new Fun module rather than added to Pervasives before 4.08 is released.

Which would be the best, Fun or Exn from #2137 ?

@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

keep the function, with a future-proof interface so that it can be fixed later.

so that 100% of the cases can be handled whenever the right tools will be there.

Is there any indication about whether/when that would happen ?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

val new_protect : acquire:(unit -> 'a) -> finally:('a -> unit) -> ('a -> 'b) -> 'b

What about finding more symmetric names for the two named arguments? acquire/release, for instance?

@gadmm gadmm force-pushed the gadmm:finally branch from 9c73cea to 2d8f444 Nov 14, 2018

gadmm added a commit to gadmm/ocaml that referenced this pull request Nov 14, 2018

Improve protect
- Treat as an error the case where two exceptions are being raised.

- Move to Fun

- Future-proof for async exceptions

See discussion at ocaml#2118
@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

What about finding more symmetric names for the two named arguments? acquire/release, for instance?

I would also be in favour of acquire, release labels, this also entails that the exception name Finally should be changed (Protect ?). Also I wonder if we should not simply call this bracket rather than protect.

Regarding the Exn vs Fun module, I'm a little bit unsure myself of which one is best. While Haskell has them in Control.Exception I tend to think that using one of Fun.{protect,bracket} and pattern matching on one of Fun.{Protect,Bracket} will read better than corresponding names in Exn.

@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Nov 14, 2018

@gasche To better focus the discussion, here is a new version with what has been said so far. I am not yet convinced to push for it though, I find it a real issue to defer to a future solution when nothing is known about it. Comments below are about everything else than async exceptions.

Both backtrace could be added to the Finally exception.

@bobot I do not see how both backtraces could be kept. I changed to raise with the original backtrace in all cases. Is this what you meant?

In any case we should ensure that this exception is by default printed in a nice way when it reaches the toplevel.

@bobot In that case isn't it best to keep the default, which prints recursively?

# raise (Finally (Finally(Not_found, Exit), Exit));;
Exception: Finally (Finally (Not_found, Pervasives.Exit), Pervasives.Exit).

We could go further and always wrap the exception coming from the finalizer.

@bobot, @alainfrisch, @lthls : I agree that it is better to fail early. I have also documented that Finally is meant as a bug like Invalid_argument. Everybody agrees that this is worth the verbosity of the new Finally exception ?

Which would be the best, Fun or Exn from #2137 ?

@dbuenzli Moving to Fun was a great idea, you remove code duplication in the systhreads version of Pervasives. Further moving to Exn makes sense: protect is already in Base.Exn, and the boilerplate of redefining get_raw_backtrace, etc, could be removed. I am not familiar with the requirements of the standard library, I got an error when I tried to compile with a direct call to Printexc.get_raw_backtrace, is this related to bootstrapping?

What about finding more symmetric names for the two named arguments? acquire/release, for instance?

@alainfrisch, I agree with acquire/release; on the other hand the names protect/finally were decided by a vote and I was hesitant to go against it. Finally naturally recalls the FINALLY statement from Modula-2, so there is some legitimacy to the name. However I find it confusing when people call this function a finalizer because this already refers to something entirely different.

Also I wonder if we should not simply call this bracket rather than protect.

@dbuenzli protect is reminiscent of unwind-protect from Common Lisp, and bracket is a small variant of unwind-protect. I find the name protect more telling.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

@bobot I do not see how both backtraces could be kept.

You could attach the backtrace in the data structure of the exception.

@alainfrisch, I agree with acquire/release; on the other hand the names protect/finally were decided by a vote and I was hesitant to go against it

We are now talking about an entirely different signature, I don't see any problem with this.

Further moving to Exn makes sense: protect is already in Base.Exn, and the boilerplate of redefining get_raw_backtrace, etc, could be removed.

I don't see that as great arguments, it's better to take the code reading experience perspective. I think Exn.{Protect,protect} would be ok (or maybe even better, I'm still undecided), but e.g. Exn.{Bracket,bracket} would read odd.

I tried to compile with a direct call to Printexc.get_raw_backtrace, is this related to bootstrapping?

More likely a dependency issue, cd to the stdlib directory and do a make depend.

Show resolved Hide resolved stdlib/fun.mli Outdated
Show resolved Hide resolved stdlib/fun.mli Outdated
Show resolved Hide resolved stdlib/fun.mli Outdated
Show resolved Hide resolved stdlib/fun.ml Outdated
@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

@bobot I do not see how both backtraces could be kept.

You could attach the backtrace in the data structure of the exception.

I now only keep the exception from the release branch, since we agree that this is the one at fault.

I think Exn.{Protect,protect} would be ok (or maybe even better, I'm still undecided)

I find it better as well. If Exn is merged, I will move it.

I tried to compile with a direct call to Printexc.get_raw_backtrace, is this related to bootstrapping?

More likely a dependency issue, cd to the stdlib directory and do a make depend.

make depend gave me an updated .depend file, but make clean && make world.opt gives me the same error:

boot/ocamlrun boot/ocamlc -g -nostdlib -I boot -use-prims runtime/primitives  -linkall -o ocaml.tmp compilerlibs/ocamlcommon.cma compilerlibs/ocamlbytecomp.cma compilerlibs/ocamltoplevel.cma toplevel/topstart.cmo
File "_none_", line 1:
Error: Required module `Stdlib__printexc' is unavailable
@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

About the name for the function, I'd prefer to reserve protect for something else (such as a function ('a -> 'b) -> ('a -> ('a, exn) result). I'd be fine with bracket, but with_resource or using would also read quite well with ~acquire/~release labels.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2018

@gadmm I think it would be much better to have the backtrace of the unexpected exception raised in ~finally.

Show resolved Hide resolved stdlib/fun.mli Outdated
Show resolved Hide resolved stdlib/fun.mli Outdated
Show resolved Hide resolved stdlib/fun.mli Outdated

@gadmm gadmm force-pushed the gadmm:finally branch from b817487 to 25b5238 Dec 4, 2018

gadmm added a commit to gadmm/ocaml that referenced this pull request Dec 4, 2018

Improve protect
- Treat as an error the case where ~finally raises an exception

- Move to Fun module

- Describe the purpose in the documentation

- Remove boilerplate

ocaml#2118
Show resolved Hide resolved stdlib/fun.mli Outdated
Show resolved Hide resolved stdlib/fun.mli Outdated
let finally_no_exn () =
try finally () with e ->
let bt = Printexc.get_raw_backtrace () in
Printexc.raise_with_backtrace (Finally_raised e) bt

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Dec 5, 2018

Contributor

I had expected to actually have exception Finally_raised of exn * backtrace ? What kind of rendering will that give us if we print the backtrace ? Will it not make it as if Finally_raised was raised by finally ?

This comment has been minimized.

Copy link
@gadmm

gadmm Dec 7, 2018

Author Contributor

Yes, that was the reason for my hesitation. With your approach, will you inspect recursively the Finally_raised exception to get to the most precise backtrace (or print them all), e.g. if you catch:

Finally_raised (Finally_raised (e, bt1), bt2)

?

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Dec 8, 2018

Contributor

Yes

I think we should err on the side of being as less confusing as possible and would thus prefer to have what I suggest. I think it's clearer: you get the exception with the backtrace up to the finally and then the Finally exception with the backtrace starting from the finally. And Yes I would print them all.

This comment has been minimized.

Copy link
@gadmm

gadmm Jan 8, 2019

Author Contributor

This makes sense; I do not have any strong opinion on this issue. While I agree with your points, it still looks somewhat clumsy to me to give Finally_raised a complicated signature just to get a list of backtraces which are all prefixes of each other; surely there is a better way. I would like to know what other developers prefer and/or if they have other ideas.

This comment has been minimized.

Copy link
@gadmm

gadmm Jan 8, 2019

Author Contributor

To develop a bit the idea behind my implementation, you only really care about the longest backtrace. If I am not mistaken, the information given by the various prefixes is redundant with the information given by the various occurrences of Re-raised at file "fun.ml", line 27, characters 36-54 in the backtrace.

Now you say that you are ready to inspect the Finally_raised exception for display purposes, but then your concern about my implementation could also be addressed by having Finally_raised (…(Finally_raised e)…) be printed in a way that reflects its special meaning, e.g. Exception: <e> under <n> Finally_raised. (The number is not informative, we could also make sure that the exception is not wrapped if it is already a Finally_raised.)

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Jan 9, 2019

Contributor

Maybe you are right that your approach is good enough. The Finally_raised exception should still maybe made more complex to hold a possible exception from work though see @edwintorok's comment.

This comment has been minimized.

Copy link
@damiendoligez

damiendoligez Jan 10, 2019

Member

Shouldn't the Finally_raised value record both exceptions and at least one backtrace? Both exceptions and both backtraces are useful because you might want to debug both your finally and your work functions.

This comment has been minimized.

Copy link
@gadmm

gadmm Jan 10, 2019

Author Contributor

@damiendoligez : see my reply to @edwintorok's comment. I propose to move the discussion to the main thread.

@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

There is still some (necessary) bikeshedding going on, but it would be nice to agree on the principles of the changes, summarised above by @gasche, to make sure that the possible hiding of serious exceptions with catchable ones in trunk gets fixed.

In particular, the only aspect of the current proposal that I argued for and that was not discussed is that we keep Fun.protect with its current signature. Three people were arguing for a different signature inspired by Haskell's bracket: @lpw25, @stedolan and I. I have changed my mind and I now think that the two are complementary. I would like to know if @lpw25 and @stedolan agree with the rationale. To summarise a long discussion, there is a use-case for protecting against catchable exceptions without going to the length of treating unexpected exceptions specially. This is a conclusion from the audit of the uses of try_finally. Here is an example of genuine protect: gadmm@e5a251e#diff-c72632776d8c5034ecdc14a264d14559, whereas uses of unwind-protect for resource management, although more common, requires greater care than just masking asynchronous exceptions. So, even if masking was implemented in a separate function with_resource, we need to keep protect to fulfil this role.

@gadmm gadmm force-pushed the gadmm:finally branch from a2dcda9 to aeb66d5 Jan 8, 2019

gadmm added a commit to gadmm/ocaml that referenced this pull request Jan 8, 2019

Improve protect
- Treat as an error the case where ~finally raises an exception

- Move to Fun module

- Describe the purpose in the documentation

- Remove boilerplate

ocaml#2118
@edwintorok

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

It would be good to preserve the original exception and backtrace raised by work (whether on its own, or by preserving both exceptions), that is more important than preserving the exception from the finalizer.
To point to a concrete example encountered in production we've carried an implementation of finally in our codebase that was swallowing the original exception, and all we got in our logs when something went wrong was Unix.Unix_error(Unix.EBADF, "close", "") from the finalizer.
This made it impossible to know how to reproduce the problem because we've lost the reason why the original exception was raised.
We've ended up changing our finally implementation like this to log and swallow the exception raised by the finalizer: https://github.com/xapi-project/stdext/pull/33/files

@damiendoligez damiendoligez added this to the 4.08 milestone Jan 10, 2019

@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

Thanks a lot for your feedback and your sharing of experience.

Here is how I understand it. You had two bugs, one of which was that you raised an exception in the finally. Swallowing the Unix.EBADF error in the finally (and maybe log or do anything else than re-raise it) is indeed what one needs to do, at all times. The goal of this PR is already to document and reveal at runtime that it is a programming error not to do so. To this end, the new implementation of protect is designed to point at this programming error, so we mainly care about showing the exception and the backtrace coming from the finally. Once this programming error is fixed, there is no issue of a main exception being lost.

As a conclusion, I am not convinced that your example demonstrate that it would be useful to preserve the original exception and backtrace raised by work, nor that it is more important than preserving the exception from the finally, because it is stems from another programming error in your code (a subtle one, but one which we have now learnt to recognise).

However, it is a good demonstration of why the current PR is important: 1) to teach the masses not to raise from the finally, and 2) to avoid an even more serious problem: that, had your program been catching this Unix_error exception, you would not have been aware of the problem at all.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2019

I'm convinced by @gadmm reasoning.

However the doc strings (sorry to insist so much on that @gadmm) do not put enough emphasis on the fact that raising in finally is a programming error. Also we need to be careful that people simply don't start to swallow arbitrary exceptions with catch all handlers in finally. I propose to tweak docstrings along the following lines:

val protect : finally:(unit -> unit) -> (unit -> 'a) -> 'a
(** [protect ~finally work] invokes [work ()] and then [finally ()]
    before [work ()] returns with its value or an exception. In the
    latter case the exception is re-raised after [finally ()].

    [finally] must never raise an exception by itself, it is a {e
    programming error} to do so. If this happens the exception
    {!Finally_raised} is raised. In order to protect [finally]
    from raising one should only catch exceptions that may be
    raised by the functions used therein; in particular one
    {e must not} use a catch all exception handler since this will
    mask asynchronous exceptions ({!Stdlib.Out_of_memory},
    {!Stdlib.Stack_overflow}, {!Sys.break}, etc.)

    [protect] can be used to enforce local invariants whether the
    function returns normally or raises an exception. However, it does
    not protect against unexpected exceptions raised inside [finally
    ()] such as {!Stdlib.Out_of_memory}, {!Stdlib.Stack_overflow}, or
    asynchronous exceptions raised by signal handlers
    (e.g. {!Sys.Break}). *)

exception Finally_raised of exn
(** [Finally_raised exn] is raised by [protect ~finally work]
    when [finally] raises an unexpected exception [exn]. This
    exception denotes a programming error it should not be
    catched except maybe at the toplevel of your program.

    Note that if this exception is raised and [work] returned by
    raising an exception that exception is lost. *)
@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

However the doc strings (sorry to insist so much on that @gadmm) do not put enough emphasis on the fact that raising in finally is a programming error.

You are right, I was thinking the same. Here is my version inspired by yours.

Also we need to be careful that people simply don't start to swallow arbitrary exceptions with catch all handlers in finally.

I tried to convey this with a more subtle phrasing, given that it is not technically forbidden if this is what they want.

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

I was reminded of this PR by something Gérard Berry said in his latest Collège de France lecture:

The most obscure error message I've seen was "Illegal error in error handler".

Back to the topic: could we please please please merge this PR in 4.08? There seems to be general agreement on this PR, and if we merge in 4.09 we'll have an incompatible change with 4.08.

@xavierleroy
Copy link
Contributor

left a comment

It looks like this PR has been reviewed and discussed to death but no one ticked the "approve" box. This isn't nice. I'm approving on behalf of the previous discussions.

Improve protect
- Treat as an error the case where ~finally raises an exception

- Move to Fun module

- Describe the purpose in the documentation

- Remove boilerplate

#2118

@gadmm gadmm force-pushed the gadmm:finally branch from 681dc11 to f68692e Feb 3, 2019

@gadmm

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2019

Thanks. I have rebased and updated the Changes file.

@nojb nojb merged commit 05ef12f into ocaml:trunk Feb 3, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

Thanks everyone!

nojb added a commit that referenced this pull request Feb 3, 2019

Merge pull request #2118 from gadmm/finally
protect: treat as an error the case where two exceptions are being raised

(cherry-picked from commit 24867b2e10db9ee24445e7e1d114891f45578d93)
@nojb

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2019

Cherry-picked to 4.08 in 70cb073.

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.