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

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

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.

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.

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).

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.

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:

• §4.1, Dolan et al., “Concurrent System Programming with Effect Handlers”, https://www.repository.cam.ac.uk/handle/1810/283239

• The discussion about the future of asynchronous exceptions at Implement Systhreads in terms of domains ocaml-multicore/ocaml-multicore#100

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.

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?

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

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.

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)

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).

@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.

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

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.

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.

@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.

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 ?

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 ?

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?

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.

@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.

@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.

@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

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.

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.

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

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.

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. *)

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.

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.

Thanks. I have rebased and updated the Changes file.

Thanks everyone!

Cherry-picked to 4.08 in 70cb073.