Reimplement Thread.exit using an exception

[xavierleroy]

[ Resurrecting #9100 because we need to do something for OCaml 5.00 ]

Previously, Thread.exit tried to behave exactly like pthread_exit(): it stops the current thread immediately, but doesn't stop the whole program as long as other threads keep running.

As discussed in #9071, this behavior is a problem for resource management ("finally" functions are not called).

As discussed during the Multicore merge review and reported in #10927, this behavior is a problem in the presence of multiple domains.

As first proposed in #9100, this PR reimplements Thread.exit by simply raising a dedicated Thread.Exit exception. This is good for resource management ("finally" functions are properly called) and simplifies the implementation tremendously.

It does introduce incompatibilities with OCaml 4. In particular, if Thread.exit is called from the main program or from a domain but not from a thread started by Thread.create, the exception Thread.Exit is not automatically caught and will abort the program / the domain, unless explicitly handled by the program.

My feeling is that with proper documentation of the incompatibilities, this reimplementation of Thread.exit is more useful than just removing the function altogether.

[damiendoligez]

LGTM

As an aside, it would be nice to know why we don't do the check_memprof_cb dance when a thread exits by uncaught exception.

[gasche]

I'm done re-reading the discussion of #9701, here is a summary of what I understand.

1. Nobody likes the current semantics of Thread.exit, and it is increasingly problematic in the Multicore runtime.
2. Raising an exception provides better semantics, but obviously it means that programmers using try .. with _ -> () can silence a Thread.exit call from deeper in their program. This is clearly documented in the present PR, but there is a risk of breaking existing code using Thread.exit (public or not). On the other hand, the exception-raising semantics is fine for most existing users.
3. @dbuenzli (and possibly @gadmm) would prefer that the function be deprecated, instead of silently changing its behavior, to encourage people to review their use-cases and check if raising an exception instead would be ok. It was even proposed to deprecate it and change the behavior as proposed at the same time.
4. @gadmm made a list of all uses of Thread.exit on opam (at the time) at https://gitlab.com/gadmm/stdlib-experiment/raw/master/other/async_audit/thread_exit
5. @jhjourdan reviewed this list in Thread.exit considered harmful #9071 (comment) , and claims that the only opam-repository program that would need fixing is Async.

There are basically two options:

1. Silently change the semantics, warn the Async people in particular and everyone in general, and hope for the best.
2. Deprecate or remove the function (so: users are forced to notice the problem), encouraging users to use raise Thread.Exit instead and fix their code so that this works.

This PR goes for option (1), which seems to have the support of @xavierleroy, @damiendoligez and @jhjourdan.

I guess that both options are probably fine.

We dearly need a document (manual chapter?) that documents the known hurdles when moving to the Multicore runtime, that people porting their code can check. Changes such as this one should be documented, and clearly we are going to forget about some of them.

[gadmm]

How about both:

• deprecating Thread.exit and change it to raising an exception in multicore, and
• deprecating Thread.exit in the 4.X branch without changing the behaviour.

?

In multicore, the worse consequences of pthread_exit are enough to justify a breaking change.

However, this does not suddenly make the exception-raising behaviour a useful design. Thread.exit was explicitly changed from raising an exception to directly exiting the thread a long time ago because this did not work in practice. So I suggest to deprecate it anyway. When raising an exception really does what the programmer intents, how about letting them just raise an exception instead?

[gadmm]

In that scenario, the Thread.Exit exception would be kept private, it is not intended to be caught by anyone else than the Thread module and there is no need to add noise to the interface.

[xavierleroy]

To add some historical background:

• A long time ago Thread.exit was implemented by raising an exception, much like in this PR. However, the exception was not exported and the fact that Thread.exit was raising an exception was not documented.
• Then someone reported that it didn't work with try ... with _ -> ...
• Then I over-reacted a bit and switched to a complicated implementation combining pthread_exit and longjmp (the poor man's exceptions)
• Instead I should have pointed out that the exception-based implementation was good for finalization and resource management (we didn't have Fun.finally back then...) and that catching all exceptions without re-raising is bad style. Then, I should have documented and exposed the exception raised by Thread.exit (so that users can catch it and finalize any way they want).

Right now, I feel I made the wrong decision back then: the exception-raising behavior is useful, and it's never too late to amend this.

[gadmm]

I am now convinced that an exception Thread.Exit understood as normal termination by the Thread module is useful, if it is given a catchable exception semantics as you do here. In this case it is natural to have the exception public, and the pitfalls will be clear to the programmer. Once it is introduced publicly, it seems that the desired behaviour can be obtained simply with raise Thread.Exit?

The breaking change needed for multicore is an orthogonal issue. What about:

• introduce Thread.Exit
• declare that Thread.exit is deprecated in favour of Thread.Exit
• change the behaviour of Thread.exit in multicore to raise e.g. Invalid_argument

?

The point is that nobody here is making an argument that the change preserves the meaning of programs, only a belief that this has no impact most of the time. So even if it happens to have no impact on a program, someone had to review it to make sure. Jacques-Henri's review on the sample that I provided showed that even well-written modern code can be affected (in the case of Async), but before that it also showed that one had to look at the code (and moreover: 1- that it was not always obvious, 2- that it sometimes revealed bugs—I am sorry but if there is a bug in my program by my fault then I want to have a look at it and see for myself how it should be fixed, not have the language developer "fix" it for me).

Another point which was not discussed enough is compatibility with OCaml 4. What will you do:

• have the new behaviour in 5, the old one in 4 (deprecated or not) ? Then you end up with incompatible behaviours. If I saw this then I would replace Thread.exit with raise Thread.Exit anyway, to recover some consistency between versions (I hope you will introduce Thread.Exit in OCaml 4).
• have the new behaviour in both? No, this breaks programs silently.

Instead, the above shows that it is possible to implement the standard warning-in-advance before a breaking change.

---

Regarding catch-alls, I think this is a red herring since any way of looking at it, it is a breaking change, but the situation is more complicated in general. This was already reflected in the quote below from the 2003 issue. It shows that even at the time of the Thread_exit exception, this was understood as a "panic" meant to traverse third-party code (even if the notion was not understood at the time), and the issues reported are those expected of panics. I agree that it might have made sense to change it to a catchable exception at the time and document the problems, without breaking code. However the opposite path was chosen to implement a poor man's panic. This was almost 20 years ago, OCaml has grown since, and it is now an “industrial-strength systems programming language” and I expect that this means a stricter backwards-compatibility story. But what is lost in the change here when programmers can simply do raise Thread.Exit instead?

From #8110:

Thread.exit implementation is therefore simply:

let exit () = raise Thread_exit

This choices makes really dangerous every "try ... with _ -> ..." usage in
thread code (yes, I know that this is a bad(TM) programming style). I've just
spent a lot of time debugging a problem like this one. Even worst most of the
"with _" weren't in my code but in other ocaml libraries code.

Another problem with this approach is the wrapping of exceptions in other
exception, a commonly used approach (e.g. Pxp_types.At exception of the PXP
parser, but I'm almost sure there is something like that in camlp4 code).

[jhjourdan]

@gadmm the issue with the solution you are proposing is that it becomes impossible to have a piece of code which both work with OCaml 4.13 (only Thread.exit) and OCaml 5.0 (only Thread.Exit). Even though the two have different behaviour, they do have close semantics, so it should be possible to write code compatible with both.

[gadmm]

To be clear the proposed solution is to introduce Thread.Exit and deprecate Thread.exit on both 4 and 5.

[gasche]

I believe that what Jacques-Henri has in mind is to (re)write the program as if Thread.exit raised a private exception, which lets you write code that is compatible with both the old and the new semantics:

1. Fix the program (when necessary) to not silence an exception that may be raised by Thread.exit
2. Once this is done, use an attribute to disable the deprecation warning on Thread.exit (if any).

With this approach, if Thread.exit raises an exception in newer versions, you get code that works correctly (without warnings) on both old and new versions of OCaml.
(I agree that being able to do this is important for some users, and a good property of a deprecation strategy.)

[gasche]

Let me detail my previous message because Guillaume tells me that it was unclear. I think that the strategy Jacques-Henri proposes is to:

• add Thread.Exit in newer OCaml versions and encourage people to use raise Thread.Exit directly
• change Thread.exit to raise Thread.Exit
• deprecate Thread.exit, with a warning that invites people to silence the warning once they checked that their code also works with the new exception-raising semantics

Unlike the proposal of Guillaume above, this lets people write code that works for both past versions of OCaml (with the old Thread.exit) and future versions of OCaml (with the new Thread.Exit).

[jhjourdan]

My "strategy" was not as clear in my mind as what you explained, @gasche, but I fully agree with what you propose.

[gadmm]

Clearer, thanks. The bit that confused me was "use an attribute to disable the deprecation warning" but I do not think it is a crucial part for what you have in mind.

Happy to see that we agree on the big picture regarding the deprecation mechanism. There are two differences in the details which should be easy to sort out:

• Keeping the old behaviour in 4.X: no need to break existing code, leave a transition period where the code works as before but has a deprecation warning (which is a good practice). They can still do the adaptations you have in mind, and in addition the new behaviour is available (we both agree to introduce Thread.Exit in both 4 and 5).
• Keeping Thread.exit deprecated in multicore: old code can be around for years, and recycling the old identifier is not useful (we both agree to encourage directly raising Thread.Exit).

I proposed that Thread.exit raises Invalid_argument rather than Thread.Exit, this difference is not what is important. I say that the suggestion to disable the warning with an attribute points to a weakness in your proposal, you say that the behaviour of Thread.exit in multicore is still important for compatibility with older OCaml versions, so (I imagine) that transitory messages on stderr are seen as unacceptable. Let's give up on the idea of Invalid_argument. Here an option to get the best of both worlds is to have a catchable exception Thread.Exit alongside a Thread.exit that raises some private exception (e.g. Thread.Deprecated_exit). This would be justified by the conceptual difference between catchable exceptions and panics, and the need to have Thread.exit deprecated since panics don't work in OCaml. (There's a proposal to implement proper panics but it's way off in terms of timeline, although this is what gave this idea.) But it's fine to have Thread.exit raise Thread.Exit if people understand why they are asked to replace it with raise Thread.Exit anyway.

Hence my final proposal adapting Gabriel's suggestion:

• add Thread.Exit in newer OCaml versions and encourage people to use raise Thread.Exit directly
• change Thread.exit in multicore to raise Thread.Exit or a private exception
• deprecate Thread.exit in both 4 and 5 plain and simple

[xavierleroy]

deprecate Thread.exit, with a warning that invites people to silence the warning once they checked that their code also works with the new exception-raising semantics

I don't think this is a correct use of deprecation warnings. Deprecation means intent to remove (at some point) so the only appropriate action from users is to stop using the deprecated thing.

I have the impression we're over-thinking the Thread.exit issue, but here some more comments, for what they are worth.

• It's getting late to introduce changes in 4.14. Declaring and handling the Thread.Exit exception is probably OK, though, because it's just an addition.
• Something should not be deprecated before the recommended replacement has been available for several releases already. (Remember the "Pervasives is now Stdlib" fiasco?) So, no, Thread.exit cannot be deprecated in 4.14.
• As an exception to this rule, Thread.exit might be deprecated in 5.00 because of the change of semantics and because 5.00 is a breaking change in many other respects.

[xavierleroy]

In an attempt to make progress, I added a "deprecated" warning to Thread.exit. If this PR is accepted, I'll submit a PR against 4.14 proposing to add the Thread.Exit exception, but without any change on Thread.exit.

[gadmm]

The outlined plan sounds good to me. I agree that it is better to not even deprecate in 4.14. Minor nitpicks on the documentation.
Edit: I meant to approve, but it is not showing up.

[gadmm]

I do not find this explanation for writing directly raise Thread.Exit convincing. You can just say that the previous behaviour was incompatible which justifies deprecating, and point out that converting to the raising form might need to review and adapt the program. In particular it might be good to point out that it is no longer guaranteed that a call to Thread.exit terminates the thread (I find the wording "unless the thread function handles the exception itself" unclear about accidental "handlings").

[abbysmal]

I took a look at the implementation and removal of the termination buffer bits and it looks good to me.
I assume there's no need to handle Exit in the uncaught_exception_handler guard as well ? (it does seems strange, but would be a slight change of behavior vs the current situation where the thread would effectively just exit.)

Edit: uncaught_exception_handler is as well a very recent feature, to be introduced in 4.14, so we are not really "breaking" anything there anyway.

[xavierleroy]

I assume there's no need to handle Exit in the uncaught_exception_handler guard as well ? (it does seems strange, but would be a slight change of behavior vs the current situation where the thread would effectively just exit.)

You mean: if the user-provided uncaught exception handler raises Thread.Exit, it should terminate the thread silently, instead of printing a backtrace and error message. That's a good point. I'm changing the implementation to do this.

[gadmm]

Let's credit Gabriel who contributed to the discussion.

First mention that the main addition is the new exception Thread.Exit?

[xavierleroy]

My cunning plan is to mention the addition of Thread.Exit in the 4.14 section of Changes. But it means I must sneak it into 4.14 first before the Changes file can be completed.