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

MPR#7903: make Thread.delay interruptible again #2306

Merged
merged 2 commits into from Mar 11, 2019

Conversation

@xavierleroy
Copy link
Contributor

commented Mar 10, 2019

This is a fix for https://caml.inria.fr/mantis/view.php?id=7903 .

In OCaml 4.07, Unix.sleepf and Thread.delay were changed so that they
would restart the sleep when interrupted by a signal (error EINTR).

The unintented consequence is that Thread.delay will not run
signal handlers until the full delay has expired. If the effect
of the handler is to raise an exception, as with Sys.catch_break,
the delay is not terminated early.

(This is specific to threaded programs, where asynchronous invocation
of signal handlers is turned off and handlers are only run at the next
leave-blocking-section. Using Unix.sleepf in a non-threaded program
doesn't show the issue.)

This commit implements a more intuitive behavior, closer to that of 4.06:
signals received during Thread.delay are handled immediately, and if
the handler returns normally, the delay is restarted with the remaining time.

A test is added in testsuite/tests/lib-threads/delayintr.ml

In OCaml 4.07, Unix.sleepf and Thread.delay were changed so that they
would restart the sleep when interrupted by a signal (error EINTR).

The unintented consequence is that Thread.delay will not run
signal handlers until the full delay has expired.  If the effect
of the handler is to raise an exception, as with Sys.catch_break,
the delay is not terminated early.

(This is specific to threaded programs, where asynchronous invocation
of signal handlers is turned off and handlers are only run at the next
leave-blocking-section.  Using Unix.sleepf in a non-threaded program
doesn't show the issue.)

This commit implements a more intuitive behavior, closer to that of 4.06:
signals received during Thread.delay are handled immediately, and if
the handler returns normally, the delay is restarted with the remaining time.

A test is added in testsuite/tests/lib-threads/delayintr.ml
@jhjourdan

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2019

The change looks good for me.

ret = select(0, NULL, NULL, NULL, &t);
/* MPR#7903: same comment as above */
caml_leave_blocking_section();

This comment has been minimized.

Copy link
@edwintorok

edwintorok Mar 10, 2019

Contributor

I think errno should be saved into a local variable prior to calling leave_blocking_section, and the saved errno should be compared to EINTR below.
Otherwise if any OCaml signal handlers are executed they may overwrite errno.

This comment has been minimized.

Copy link
@xavierleroy

xavierleroy Mar 11, 2019

Author Contributor

caml_leave_blocking_section carefully preserves errno in a local variable and restores it before returning, for the reason you mention. So the code is correct as written.

is handled in OCaml, we should run its handler now,
not at the end of the full sleep duration. Leaving the blocking
section and re-entering it does the job. */
caml_leave_blocking_section();

This comment has been minimized.

Copy link
@edwintorok

edwintorok Mar 10, 2019

Contributor

See my comment below about errno.

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

Thanks for the very quick feedback. CI precheck is happy. Merging now.

@xavierleroy xavierleroy merged commit 187ceb6 into ocaml:trunk Mar 11, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@xavierleroy xavierleroy deleted the xavierleroy:mpr7903 branch Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.