The way I understand the previous code, it already polled before retrying the system call, inside caml_enter_blocking_section and caml_leave_blocking_section. The new version ensures that channels are unlocked when signals are called, though. Am I right?

Yes, that's correct. I'll update the PR description to make this clearer.

If a signal is handled immediately during a blocking section, then any progress made by that blocking section is discarded if the handler raises. Specifically, a signal may occur after a system call has successfully returned but before the blocking section has ended, and its return value was lost. There is in general no way to recover from this in the case of say, writing to a pipe.

I do not see exactly what situations you are referring to, but could these situations also be dealt with explicit clean-up after call to a caml_{enter,leave}_blocking_section_exn function as the ones proposed at #8997?

Now, the clean-up approach cannot deal with the problem of unlocking channels, but I think that recursive mutexes might be a good solution to locking twice in this situation, which would simplify the code.

Using recursive mutexes here would introduce subtle bugs. The arguments to e.g. caml_read_fd would be invalidated by a recursive use of the same channel. This is hard to spot and hard to fix.

It is much simpler to prevent signal handlers running between the accesses to chan in io.c and the call to read.

Another question: it is not clear to me that the (non-)SA_RESTART behaviour of signal is portable. Linux signal man page contradicts the glibc manual about glibc's own behaviour (which is also different from the Linux system call)... I did not find anything in POSIX. I wonder if it is better to use sigaction with explicit omission of SA_RESTART.

I think we unconditionally use sigaction? I agree signal is dicey here, but I'm not sure where it's used.

Another question: it is not clear to me that the (non-)SA_RESTART behaviour of signal is portable. Linux signal man page contradicts the glibc manual about glibc's own behaviour (which is also different from the Linux system call)... I did not find anything in POSIX. I wonder if it is better to use sigaction with explicit omission of SA_RESTART.

I think we unconditionally use sigaction? I agree signal is dicey here, but I'm not sure where it's used.

Ah, there's some code that uses signal if POSIX_SIGNALS isn't defined. I think this might only be reachable on Windows?

I'm sorry I haven't had time for a proper review, and will not until next week. I just wanted to say I'm glad to see this work, as it looks simpler than #1128 yet should fix several issues.

Concerning the lack of atomicity pointed out by @jhjourdan : the current implementation has similar problems, and many other problems that this PR fixes, so 1- it's still a big step forward, and 2- once this is reviewed and merged, we can think of addressing the atomicity problem with the self-pipe trick.

@gadmm, @jhjourdan This patch makes signal handling have the following properties, which do not currently hold:

• Signal handlers are run only at polling points. In particular, signal handlers do not interrupt arbitrary C code
• Signal handlers are never run while holding channel locks
• Signal handlers can do channel I/O and GC without corrupting state

There is an additional nice property, which as @xavierleroy points out did not hold before this patch and does not hold afterwards:

• The runtime should never wait an unbounded time before handling a signal

The reason this does not hold is the same before and after this patch: a race between the last check for signals and entering a blocking operation. I have some ideas for a generic fix for this, but they'll be a separate PR.

(In other words: the current runtime has both safety and liveness issues. This patch fixes only the former)

@jhjourdan

Apart from that, I am not quite sure I understand why #8691 simplified this PR that much. Which is the OCaml runtime function which checked for signals in #1128 but no longer does so?

There are broadly two coding styles that allow correct handling of asynchronous signals.

1. Write code that can be interrupted at any point, and try to ensure that it safely cleans up resources / locks / etc. if it is ever interrupted.
2. Write code that polls for signals at designated points where cleanup is easy, and try to ensure that it always reaches such a point shortly after a signal is delivered.

#1128 attempted style 1, which required extremely careful and fragile coding, and needed fairly extensive changes to the internal interfaces of io.c / unix.c. This patch attempts style 2, which is much simpler.

At the time of writing #1128, I didn't see style 2 as an option, since almost any interaction with the runtime could in principle run signal handlers. Since #8691, controlling when signals may run seems easier. (Having said that, it's likely that style 2 might have worked before then, by being careful about which runtime functions were called)

Ah, there's some code that uses signal if POSIX_SIGNALS isn't defined. I think this might only be reachable on Windows?

This is indeed what must have confused me. Thanks for checking.

My thoughts about the possible guarantees, and the replies to previous points, all in one message (the only one).

In this PR there are two changes that I think are better to discuss separately:

1. No longer interrupt blocking sections by running OCaml signal handlers from C signal handlers.
2. Unlock channels before checking for pending signals, and publish the API to allow users to write this kind of code.

My concerns are mainly about the API part of 2.

1. No longer interrupt blocking sections by running OCaml code inside signal handlers

The question is what should it be replaced with (either now if deemed necessary, or later). The titles of the PRs let us think (or me at least) that this would involve handling EINTR inside more blocking sections than before, and this is what I have been thinking about. The current PR does not change where EINTR is handled.

Taking a step back (these notes are from when I was reviewing the previous PR), there are several things achieved (in spirit at least) by the current broken implementation:

The user does not need to handle EINTR

This has been requested at #7508:

System call wrappers provided in the standard library should be retried automatically when they fail with EINTR, to relieve application code from the burden of doing so.

referring to by a similar python proposal (https://www.python.org/dev/peps/pep-0475/). This sounds reasonable, independently of the next points. Currently, EINTR is only handled for read and write in unix.c.

Guarantee that signals are handled promptly enough

The race between the signal and the start of the system call has been discussed at length. A couple of points though:

• the race is special, because if the signal can be re-sent, then the race is no longer an issue. This is true for sigint where the user can press Ctrl-C again, less obvious for signals like sigterm (there might be workarounds though by waiting for the signal on a separate thread). I believe one could live with the race if necessary. (This is a "do not fight POSIX" approach.)
• the self-pipe trick is interesting because in addition to avoiding the race, it becomes possible to interrupt the blocking sections without having to rely on signals (which is cumbersome in the presence of threads).

Blocking sections in general can be interrupted, not merely system calls able to return EINTR

This point has not been raised before, but I wanted to raise it because the question of whether OCaml will support cleanly interrupting blocking calls in general is going to influence how the masking API (#8961) is designed (details better discussed elsewhere). As a reference point, blocking calls in GHC (relevant here in the context of their experiment with masking) are interruptible in general, for instance takeMVar and putMVar are interruptible (whereas Mutex.lock and Condition.wait are not interruptible in OCaml). (For POSIX threads it seems that one could use semaphores, for instance, with sem_wait returning EINTR.)

I do not believe it is necessary to do any of this as part of this PR (because this did not work reliably up to now, and has always been disabled under systhreads), but I wonder what was the original purpose of the non-systhreads behaviour (e.g. make the toplevel more responsive to sigint?), and whether this could inform what replacement is desirable in the short term (if any).

2. Unlock channels before checking for pending signals, and publish the API to allow users to write this kind of code

The way I understand it now, the API changes mix two different goals:

1. one the one hand, make public what is needed to implement unlocking as in this PR,
2. on the other hand, trying to deal with the consistency of state and the release of resources should an async exception arise.

Unlocking

This PR allows using the same channel in a signal handler and in the main code (typically stdout). Note: this costs the atomicity for the channel operation that previously was guaranteed by the lock. I am not sure what impact this is going to have. This includes being interrupted by a signal (whose handler need not use the channel), only to yield to another thread that wants to use the same channel. (Recursive mutexes would avoid this but as you note this looks at least as complicated to implement.)

It is still delicate to implement this, while I am not convinced that this use-case should be supported, at least in the general case. In general, the programmer could be told to either avoid sharing a mutex between async callbacks and the main code, or to have the critical sections be uninterruptible.

So, as far as the API proposal is concerned, I am not convinced that it is good to offer the functions caml_enter_blocking_section_no_pending and caml_check_pending_actions in the public API (given other reasons I mentioned previously).

In addition, while the proposed implementation of unlocking is good enough, an alternative I have thought about would be to use an errorcheck mutex so as to be able to raise a nice exception in case the signal handler tries to re-use a channel (and to document this fact). Then this would preserve the atomicity.

State/resource safety

Signal handlers are run only at polling points. In particular, signal handlers do not interrupt arbitrary C code

This concerns the change in caml_leave_blocking_section, which currently polls to raise the interrupt as soon as possible. So there is #8997 which already proposes caml_enter/leave_blocking_section_exn variants for being able to clean-up after an exception, and #8961 that gives the same behaviour as yours under the appropriate mask. (There is a nice example usage with the implementation of caml_sys_open in sys.c which is leaky and this is bad, but the proposed caml_leave_blocking_section_exn is meant to fix this.)

Changing the polling behaviour of caml_leave_blocking_section is a radical move that I would not have dared try. I would not know how to evaluate the consequences. I think it is good that signals are checked first thing, as there are not that many polling points. I also believe that the change is unnecessary and overkill: I am missing what is different between the problems you see with polling there, and the usual way ML interrupts invalidate state.

What worries me is if the implication is that it should be the responsibility of the person who polls to ensure the consistency of state, because this is not the correct way to deal with the problem. Instead, it is the responsibility of the person catching the interrupt to make sure that no invalid state escapes. (I have documented at https://gitlab.com/gadmm/memprof-limits/-/blob/master/doc/recovering.md this, which is the only way I know to safely handle interrupts.) To take your example of writing to a pipe, one should not care about losing the result of the syscall, because the pipe itself should disappear before the interrupt is caught, and never be seen again.

(The alternative is to add a new internal function caml_leave_blocking_section_no_pending for the moment.)

In short, what disturbs me with the unlocking and the moving of polling to the topmost callers is that this tries to treat asynchronous callbacks as if they were synchronous. Moreover, the API change means that this is going to be considered a good pattern for users to follow. But one has to make the burden of asynchronous callbacks bearable, and I am worried that this might not be a viable strategy at scale. It is much simpler to consider that users are not entitled to share locks between the asynchronous callbacks and the rest of the program.

I have updated #8997, which deals with a few leaks in the runtime by explicitly returning the exception in caml_enter/leave_blocking_section and applying standard resource-management patterns. From this I do not see how no longer polling in caml_leave_blocking_section would make things much simpler. Next I will try to apply the recipe in #8997 to experiment with the alternative I proposed for the second part of your PR (where one gets an error when locking recursively).

• Resource-safe C interface, part 2 #8997 now fixes the leaks in io.c. It is worth it to guarantee that the channel state remains valid at all times since it is simple to implement. For comparison with the current PR, see 8971cec1 for caml_read_fd and 6e0152bb for caml_write_fd. They rely on boilerplate which is also used to fix other bugs (67250e56).

  This approach is incompatible with unlocking the channels before running the asynchronous callbacks, in which case a larger change like yours is necessary (it is a different coding style as you describe). The upside of this coding style is that the fixes at Resource-safe C interface, part 2 #8997 are mechanical and change the current behaviour the least, and the API proposal is accordingly simpler.

• In addition, at Make systhread mutexes errorcheck #9757 I implemented the raising of an exception if a mutex is being locked recursively. (I think it is worthwhile to consider it separately, because it fixes deadlocks with Mutex too.)

I found it interesting to discover these issues with state invalidation with your work around this PR. I believe that with #8997 and #9757, the approach in the second part of this PR is not necessarily needed, and it comes down to whether one wants to preserve the current behaviour of programs under systhread or under no systhread:

• Choose current no-systhread behaviour: read and writes can be interleaved with read/writes from finalisers and signal handlers (and thus other threads too) -> implement unlocking as done here (in addition to Resource-safe C interface, part 2 #8997 and Make systhread mutexes errorcheck #9757, which still fix separate issues).
• Choose current systhread behaviour: reads and writes are atomic -> in addition to Resource-safe C interface, part 2 #8997 and Make systhread mutexes errorcheck #9757, one must also bring the non-systhread behaviour in line with the systhread behaviour. I see this done by providing default implementations for the caml_channel_mutex_* hooks that perform locking using a boolean flag.

@gadmm I'll try to respond to as many of your points as I can here:

1. No longer interrupt blocking sections by running OCaml code inside signal handlers

The user does not need to handle EINTR
this would involve handling EINTR inside more blocking sections than before

This is a misconception. Currently, blocking sections return EINTR:

• when interrupted by a signal, if systhreads is not linked
• when interrupted by a signal whose handler does not raise (whether or not systhreads is linked)

So, with this patch the set of EINTR-returning places does not change. The difference is that EINTR is used to signal interrupts even for signal handlers which raise, rather than using the inherently-unsafe approach of running OCaml code in a Unix signal handler.

Guarantee that signals are handled promptly enough

I agree with your points here. I'd like to discuss them in a separate PR.

Blocking sections in general can be interrupted, not merely system calls able to return EINTR
As a reference point, blocking calls in GHC (relevant here in the context of their experiment with masking) are interruptible in general

GHC implements interruptible blocking C calls in exactly the way proposed by this patch - to interrupt, it causes a system call to return EINTR. See the GHC manual, section 10.2.5. There is no other way to make arbitrary blocking sections interruptible in general.

2. Unlock channels before checking for pending signals, and publish the API to allow users to write this kind of code

Unlocking
Note: this costs the atomicity for the channel operation that previously was guaranteed by the lock

In what operation is atomicity lost? For most of these operations, partial results are returned without checking for signals, so I'm not sure there can be I/O interleaved with signal handling.

Besides, I am not convinced there was any such guarantee! The implementations used a lock internally to avoid corruption, but no guarantee was made anywhere. Many operations on channels (e.g. print_endline) in fact involve multiple lock-unlock cycles, and can interleave output when used from multiple threads.

an alternative I have thought about would be to use an errorcheck mutex so as to be able to raise a nice exception in case the signal handler tries to re-use a channel (and to document this fact).

I think this is a bad idea. Printing a message to stdout or stderr is a very common use of signal handlers, and under your proposal this operation could nondeterministically raise a mysterious exception about mutexes.

Your approach in #8997 + #9757 still seems to run arbitrary user code while holding a channel lock. What happens if a signal handler prints to stderr, while the main program is also doing so? What happens if a signal handler closes stderr? Have you tested your code with my example program from #1128?

While reviewing, I took the liberty of pushing a small Win32 fix directly to the branch (commit f4bf623). Since Win32 has no notion of signal that can interrupt a system call, the change is mostly cosmetic: replace caml_enter_blocking_section with caml_enter_blocking_section_no_pending, for consistency with what was done in runtime/unix.c.

@stedolan, for 1. I was unclear, the first point was not meant to be about a property we might lose but one that was requested elsewhere. I think we are in agreement overall.

For 2., I had in mind operations like caml_input_value, caml_output_value and caml_input_scan_line which are atomic currently (and thus Stdlib.input_value and Stdlib.output_value). Your point is that for instance Stdlib.input_line is already buggy if somebody else accesses the channel concurrently, so it does not matter if caml_input_scan_line loses atomicity. I agree, and I was not making this behaviour change critical (because there is good chance that the code this would affect is already buggy).

Printing a message to stdout or stderr is a very common use of signal handlers, and under your proposal this operation could nondeterministically raise a mysterious exception about mutexes.

Currently you could have mysterious deadlocks (with systhreads) or crashes (without it) instead. My point of view is that these concurrent accesses do bad things in general and would generally be considered as bugs, so it is better to fail early with an exception. You are interested in a use-case where the bad thing is acceptable. The approach at #9757 is biased towards fast bug reporting, but there are workarounds if the programmer insists, such as (AFAIU) creating a separate channel from a dup of stdout/stderr.

But, I agree that the additional guarantee of not running callbacks while holding a lock can be sensible in this case (and the specification "use a lock internally to avoid corruption, but make no guarantee of atomicity"). My point of view is mainly from thinking about what is a good API for the programmer, and I am not convinced by the public API and behaviour changes. In particular, #8997 shows that polling inside caml_leave_blocking_section is not an obstacle to ensuring the consistency of state in general, and your issue with it is rather specific to what you are trying to achieve and to me does not warrant such a large change in polling behaviour.

you could introduce a specific function caml_leave_blocking_section_no_pending, but instead you change the behaviour for everybody who ever acquires the runtime lock.

I can sympathize with this argument. Perhaps a new caml_leave_blocking_section_no_pending function could be introduced by symmetry with caml_enter_blocking_section_no_pending and to minimize changes?

I would still like this PR to be merged well in time for release 4.12. #8961 and #8997 look like longer-term projects to me, of the kind that will not be reviewed in time for 4.12.

you could introduce a specific function caml_leave_blocking_section_no_pending, but instead you change the behaviour for everybody who ever acquires the runtime lock.

I can sympathize with this argument. Perhaps a new caml_leave_blocking_section_no_pending function could be introduced by symmetry with caml_enter_blocking_section_no_pending and to minimize changes?

The thing is, it's not like this is a change that is likely to break programs. This will simply delay some signals a little more (until the next polling point). But since polling point (should be) are inserted regularly, the introduced delay should be too long.

In particular, #8997 shows that polling inside caml_leave_blocking_section is not an obstacle to ensuring the consistency of state in general

#8997 introduces caml_enter_blocking_section_noexn and caml_leave_blocking_section_noexn (with the semantics of caml_enter_blocking_section_no_pending and caml_leave_blocking_section here). From what I can tell, they are introduced because polling inside caml_leave_blocking_section is an obstacle to ensuring the consistency of state in general.

In the cases where #8997 uses the polling variant of leave_blocking_section, it risks discarding an exception from a failed operation. (Perhaps you don't care about this case, but this case is what my argument above was about).

I have yet to see a program whose behaviour is improved by polling in leave_blocking_section, but I've seen several where the polling results in a bug. The usual argument against removing a polling point from a function is that it might cause unbounded delay if a program repeatedly calls it, but that can't happen here: any code that calls caml_leave_blocking_section repeatedly must also call caml_enter_blocking_section repeatedly, and that's a better place to poll.

In particular, #8997 shows that polling inside caml_leave_blocking_section is not an obstacle to ensuring the consistency of state in general

#8997 introduces caml_enter_blocking_section_noexn and caml_leave_blocking_section_noexn (with the semantics of caml_enter_blocking_section_no_pending and caml_leave_blocking_section here).

Yes, we agree that sometimes we need this behaviour! This is the point of #8961 (not yet ready to land).

From what I can tell, they are introduced because polling inside caml_leave_blocking_section is an obstacle to ensuring the consistency of state in general.

Not really, this is about avoiding leaks rather than invalidating the state of values that are still in use, and it is exclusively used in the release path (I did not remove polling locations). The transformation used at #8997 is systematic and explains why you need them in the release path. If you used the raising version, then it tells you to use the _exn variant, test for an exception, and clean-up, that is... proceed with what you were doing in the release path (re-release the lock, re-test for an exception, etc.). From this, one quickly concludes that signals must be masked during release, as a general principle. Also, while I applied a systematic transformation that preserved all polling locations, I agree that there are places where I could have simplified the code if I removed polling at some places (both when acquiring and releasing the lock). My objection was rather that with masking and the _exn variants, the user eventually will have enough tools, and it is better to keep the public API as simple as possible. If they have to be superseded, then it is easier if they remain internal.

In the cases where #8997 uses the polling variant of leave_blocking_section, it risks discarding an exception from a failed operation. (Perhaps you don't care about this case, but this case is what my argument above was about).

True, I considered the error from the failed operation as discardable. Your point is quite subtle and interesting. If we apply the general rule not to drop exceptions, I agree that one cannot poll when acquiring the lock. But this generally applies to exceptions that we do not know where they come from. Here we know what the exception is, and we could consider that it has not been raised yet (one could permute the polling and the conversion the error code into the exception). At least this is my reasoning here.

I have yet to see a program whose behaviour is improved by polling in leave_blocking_section, but I've seen several where the polling results in a bug. The usual argument against removing a polling point from a function is that it might cause unbounded delay if a program repeatedly calls it, but that can't happen here: any code that calls caml_leave_blocking_section repeatedly must also call caml_enter_blocking_section repeatedly, and that's a better place to poll.

I agree that this "usual argument" does not apply and I like the argument. Rather, I am very cautious about this sort of radical change. My approach is to not assume that I can know what code exists in the wild; here maybe some code is already poor in polling locations. And as Jacques-Henri recalls, polling should be inserted regularly. Right when acquiring the lock has the most chance to be helpful.

Could we please make progress towards merging this PR? The points of disagreement have been presented in details, and look small to me, so it should be possible to move forward.

I think the issue with caml_leave_blocking_section is no longer blocking. While the arguments given initially seem incorrect to me, I agree with @stedolan's new explanation about the change being unable to cause unbounded delays. I am still uncomfortable with such a radical change, but I leave it to @stedolan's appreciation.

(I think the change deserved such a discussion. Some of the disagreement can probably be explained by the fact that we have different ideas of how frequently the runtime should poll, which has never been specified.)

Whether we should be wary of adding too many and redundant functions to the public API is also a matter of taste to some degree, so I leave it to @stedolan's appreciation.

Since Win32 has no notion of signal that can interrupt a system call, the change is mostly cosmetic

It is worth noting that according to @stedolan's link, it is possible to interrupt the system call:

[Vista and later only] The RTS calls the Win32 function CancelSynchronousIo, which will cause a blocking I/O operation to return with the error ERROR_OPERATION_ABORTED.

It is worth noting that according to @stedolan's link, it is possible to interrupt the system call: [Vista and later only] The RTS calls the Win32 function CancelSynchronousIo, which will cause a blocking I/O operation to return with the error ERROR_OPERATION_ABORTED.

Good to know! I see there's also WSACancelBlockingCall for socket I/O. I'll think about how to take this into account in runtime/win32.c. It shouldn't be hard to return the magic "Io_interrupted" result in case of cancellation. What is less clear to me is whether and how cancellation should be mapped to a signal so that it can be handled using the signal handler machinery.

Here is a consequence of not polling in caml_leave_blocking_section (AFAIU).

Currently, if a system call gives EINTR (other than caml_fd_read and caml_fd_write from this PR), then caml_leave_blocking_section will perform the check for signals. After this PR, it might raise Sys_error or Unix.Unix_error instead. Examples include caml_ml_close_channel and caml_sys_open from the runtime, wrappers from the Unix module, and probably 3rd-party code in the wild.

Now, user code might typically perform some clean-up before handling the exception. In the current incorrect way to do clean-up available to the OCaml programmer (e.g. Fun.protect or manual try/re-raise), a signal handler being executed inside the clean-up code and raising is likely to cause a leak. Currently, this is a race condition that happens rarely (or so we hope). By raising Sys_error or Unix.Unix_error while a signal is pending, these leaks no longer result from a race condition but from the way the system call wrappers are implemented.

Furthermore, if it was possible to run clean-up code correctly, that is with pending signals masked, then the execution of the signal handler will instead be delayed until after all the clean-up is done. One consequence is that if Sys_error or Unix.Unix_error was not handled specifically, then the task will see an uncaught Sys_error or Unix.Unix_error exception. Yet we have seen code that treats the exception arising from the signal handler specially (for instance taking the decision to respawn the task instead of aborting, and other examples at #8873 (comment)). In this case, the execution of the signal handler might happen too late. (This is also true, in current OCaml, if the code does no clean-up or if clean-up does not poll.)

Thus, even if we believe that the program will poll soon enough, it might not be where we want it. A solution is that the users should first check for EINTR and poll, but polling in caml_leave_blocking_section has been doing that for them.

Do you agree with this analysis?

I've rebased the history into a reasonable series of commits, and added tests for various cases of signal handling, GC and I/O. These tests uncovered a couple more bugs (e.g. out_channel_length didn't lock the channel) so there are a few extra changes to io.c.

I'd appreciate someone having a quick read of the io.c changes, but otherwise I consider this ready to merge (let's see if CI agrees...)