Skip to content

Ensure signals are handled before Unix.kill returns#9802

Merged
stedolan merged 1 commit intoocaml:trunkfrom
stedolan:poll-in-unix-kill
Jul 30, 2020
Merged

Ensure signals are handled before Unix.kill returns#9802
stedolan merged 1 commit intoocaml:trunkfrom
stedolan:poll-in-unix-kill

Conversation

@stedolan
Copy link
Copy Markdown
Contributor

POSIX does not make very many guarantees about when signals are handled, but one guarantee that it does make is that if a process sends itself a signal (using e.g. kill(getpid(), ...)), then signals will be handled before kill returns.

This is currently not true of OCaml's Unix.kill, this patch makes it so.

(This came up during testing of #9722, but is a separate issue)

@stedolan stedolan force-pushed the poll-in-unix-kill branch 2 times, most recently from b5d00c7 to 8e595b9 Compare July 27, 2020 12:56
Copy link
Copy Markdown
Contributor

@jhjourdan jhjourdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is trivial, looks good to me.

@stedolan
Copy link
Copy Markdown
Contributor Author

stedolan commented Jul 27, 2020

Searching through the standard, I see that POSIX makes the same stipulation about the other kills that can cause a signal to become pending (sigprocmask, pthread_sigmask), so I'll fix those too.

@stedolan
Copy link
Copy Markdown
Contributor Author

@jhjourdan it turns out that your test from #9027 (tests/callback/signals_alloc.ml) is explicitly testing that kill does not run the signal handler. Is this intentional?

@stedolan stedolan force-pushed the poll-in-unix-kill branch from 8e595b9 to 02c686d Compare July 27, 2020 15:51
@stedolan
Copy link
Copy Markdown
Contributor Author

I've updated the tests to avoid assuming this behaviour of kill, and added a test for sigprocmask. (This test passes with no code change due to a poll in caml_leave_blocking_section, but I want the test there before #9722 is merged)

@jhjourdan
Copy link
Copy Markdown
Contributor

jhjourdan commented Jul 27, 2020

@jhjourdan it turns out that your test from #9027 (tests/callback/signals_alloc.ml) is explicitly testing that kill does not run the signal handler. Is this intentional?

Yes, it is. The point of this test is to check that allocations do indeed poll, so polling should not occur in when raising the signal. Perhaps you could use the same strategy as in the other test?

@stedolan stedolan force-pushed the poll-in-unix-kill branch from 02c686d to ee4f765 Compare July 28, 2020 09:19
@stedolan
Copy link
Copy Markdown
Contributor Author

Yes, it is. The point of this test is to check that allocations do indeed poll, so polling should not occur in when raising the signal. Perhaps you could use the same strategy as in the other test?

Done.

@stedolan stedolan force-pushed the poll-in-unix-kill branch 2 times, most recently from a9691cb to 4c4629d Compare July 28, 2020 09:24
@stedolan stedolan mentioned this pull request Jul 28, 2020
1 task
@stedolan stedolan force-pushed the poll-in-unix-kill branch from 4c4629d to ca34559 Compare July 28, 2020 17:31
@stedolan stedolan merged commit 5b4b834 into ocaml:trunk Jul 30, 2020
@nojb
Copy link
Copy Markdown
Contributor

nojb commented Jul 30, 2020

This PR has broken the INRIA CI:

The error (visible in the above logs) is

Fatal error: exception File "signals_alloc.ml", line 27, characters 2-8: Assertion failed

@stedolan
Copy link
Copy Markdown
Contributor Author

That is weird. I'm running a precheck build with a more verbose version of the test to see if I can see what's going on.

@stedolan
Copy link
Copy Markdown
Contributor Author

Possible fix in #9814

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants