summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
-rw-r--r--ChangeLog29
-rw-r--r--eval.c2
-rw-r--r--process.c48
-rw-r--r--thread.c4
-rw-r--r--thread_pthread.c158
-rw-r--r--thread_win32.c8
-rw-r--r--vm_core.h2
7 files changed, 134 insertions, 117 deletions
diff --git a/ChangeLog b/ChangeLog
index e5a3cf0..f0f3f9e 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,32 @@
+Fri Aug 14 18:43:11 2015 Eric Wong <e@80x24.org>
+
+ * process.c (close_unless_reserved): add extra check
+ (dup2_with_divert): remove
+ (redirect_dup2): use dup2 without divert
+ (before_exec_non_async_signal_safe): adjust call + comment
+ (rb_f_exec): stop timer thread for all OSes
+ (rb_exec_without_timer_thread): remove
+ * eval.c (ruby_cleanup): adjust call
+ * thread.c (rb_thread_stop_timer_thread): always close pipes
+ * thread_pthread.c (struct timer_thread_pipe): add writing field,
+ mark owner_process volatile for signal handlers
+ (rb_thread_wakeup_timer_thread_fd): check valid FD
+ (rb_thread_wakeup_timer_thread): set writing flag to prevent close
+ (rb_thread_wakeup_timer_thread_low): ditto
+ (CLOSE_INVALIDATE): new macro
+ (close_invalidate): new function
+ (close_communication_pipe): removed
+ (setup_communication_pipe_internal): make errors non-fatal
+ (setup_communication_pipe): ditto
+ (thread_timer): close reading ends inside timer thread
+ (rb_thread_create_timer_thread): make errors non-fatal
+ (native_stop_timer_thread): close write ends only, always,
+ wait for signal handlers to finish
+ (rb_divert_reserved_fd): remove
+ * thread_win32.c (native_stop_timer_thread): adjust (untested)
+ (rb_divert_reserved_fd): remove
+ * vm_core.h: adjust prototype
+
Fri Aug 14 18:40:43 2015 Nobuyoshi Nakada <nobu@ruby-lang.org>
* ext/win32/lib/win32/registry.rb (API#SetValue): add terminator
diff --git a/eval.c b/eval.c
index af88e4b..f598525 100644
--- a/eval.c
+++ b/eval.c
@@ -223,7 +223,7 @@ ruby_cleanup(volatile int ex)
/* unlock again if finalizer took mutexes. */
rb_threadptr_unlock_all_locking_mutexes(GET_THREAD());
TH_POP_TAG();
- rb_thread_stop_timer_thread(1);
+ rb_thread_stop_timer_thread();
ruby_vm_destruct(GET_VM());
if (state) ruby_default_signal(state);
diff --git a/process.c b/process.c
index cdc9692..206a445 100644
--- a/process.c
+++ b/process.c
@@ -297,24 +297,14 @@ extern ID ruby_static_id_status;
static inline int
close_unless_reserved(int fd)
{
- /* Do nothing to the reserved fd because it should be closed in exec(2)
- due to the O_CLOEXEC or FD_CLOEXEC flag. */
+ /* We should not have reserved FDs at this point */
if (rb_reserved_fd_p(fd)) { /* async-signal-safe */
+ rb_async_bug_errno("BUG timer thread still running", 0 /* EDOOFUS */);
return 0;
}
return close(fd); /* async-signal-safe */
}
-static inline int
-dup2_with_divert(int oldfd, int newfd)
-{
- if (rb_divert_reserved_fd(newfd) == -1) { /* async-signal-safe if no error occurred */
- return -1;
- } else {
- return dup2(oldfd, newfd); /* async-signal-safe */
- }
-}
-
/*#define DEBUG_REDIRECT*/
#if defined(DEBUG_REDIRECT)
@@ -354,7 +344,7 @@ static int
redirect_dup2(int oldfd, int newfd)
{
int ret;
- ret = dup2_with_divert(oldfd, newfd);
+ ret = dup2(oldfd, newfd);
ttyprintf("dup2(%d, %d) => %d\n", oldfd, newfd, ret);
return ret;
}
@@ -388,7 +378,7 @@ parent_redirect_close(int fd)
#else
#define redirect_dup(oldfd) dup(oldfd)
-#define redirect_dup2(oldfd, newfd) dup2_with_divert((oldfd), (newfd))
+#define redirect_dup2(oldfd, newfd) dup2((oldfd), (newfd))
#define redirect_close(fd) close_unless_reserved(fd)
#define parent_redirect_open(pathname, flags, perm) rb_cloexec_open((pathname), (flags), (perm))
#define parent_redirect_close(fd) close_unless_reserved(fd)
@@ -1151,8 +1141,10 @@ before_exec_non_async_signal_safe(void)
* internal threads temporary. [ruby-core:10583]
* This is also true on Haiku. It returns Errno::EPERM against exec()
* in multiple threads.
+ *
+ * Nowadays, we always stop the timer thread completely to allow redirects.
*/
- rb_thread_stop_timer_thread(0);
+ rb_thread_stop_timer_thread();
}
static void
@@ -2472,10 +2464,6 @@ rb_execarg_parent_end(VALUE execarg_obj)
RB_GC_GUARD(execarg_obj);
}
-#if defined(__APPLE__) || defined(__HAIKU__)
-static int rb_exec_without_timer_thread(const struct rb_execarg *eargp, char *errmsg, size_t errmsg_buflen);
-#endif
-
/*
* call-seq:
* exec([env,] command... [,options])
@@ -2559,16 +2547,14 @@ rb_f_exec(int argc, const VALUE *argv)
execarg_obj = rb_execarg_new(argc, argv, TRUE);
eargp = rb_execarg_get(execarg_obj);
+ before_exec(); /* stop timer thread before redirects */
rb_execarg_parent_start(execarg_obj);
fail_str = eargp->use_shell ? eargp->invoke.sh.shell_script : eargp->invoke.cmd.command_name;
-#if defined(__APPLE__) || defined(__HAIKU__)
- rb_exec_without_timer_thread(eargp, errmsg, sizeof(errmsg));
-#else
- before_exec_async_signal_safe(); /* async-signal-safe */
rb_exec_async_signal_safe(eargp, errmsg, sizeof(errmsg));
- preserving_errno(after_exec_async_signal_safe()); /* async-signal-safe */
-#endif
+
+ preserving_errno(after_exec()); /* restart timer thread */
+
RB_GC_GUARD(execarg_obj);
if (errmsg[0])
rb_sys_fail(errmsg);
@@ -3076,18 +3062,6 @@ failure:
return -1;
}
-#if defined(__APPLE__) || defined(__HAIKU__)
-static int
-rb_exec_without_timer_thread(const struct rb_execarg *eargp, char *errmsg, size_t errmsg_buflen)
-{
- int ret;
- before_exec();
- ret = rb_exec_async_signal_safe(eargp, errmsg, errmsg_buflen); /* hopefully async-signal-safe */
- preserving_errno(after_exec()); /* not async-signal-safe because it calls rb_thread_start_timer_thread. */
- return ret;
-}
-#endif
-
#ifdef HAVE_WORKING_FORK
/* This function should be async-signal-safe. Hopefully it is. */
static int
diff --git a/thread.c b/thread.c
index 1d39d16..f64a45d 100644
--- a/thread.c
+++ b/thread.c
@@ -3864,9 +3864,9 @@ timer_thread_function(void *arg)
}
void
-rb_thread_stop_timer_thread(int close_anyway)
+rb_thread_stop_timer_thread(void)
{
- if (TIMER_THREAD_CREATED_P() && native_stop_timer_thread(close_anyway)) {
+ if (TIMER_THREAD_CREATED_P() && native_stop_timer_thread()) {
native_reset_timer_thread();
}
}
diff --git a/thread_pthread.c b/thread_pthread.c
index 26fe9ff..8bc5bac 100644
--- a/thread_pthread.c
+++ b/thread_pthread.c
@@ -1270,9 +1270,16 @@ static int check_signal_thread_list(void) { return 0; }
#if USE_SLEEPY_TIMER_THREAD
static struct {
+ /*
+ * Read end of each pipe is closed inside timer thread for shutdown
+ * Write ends are closed by a normal Ruby thread during shutdown
+ */
int normal[2];
int low[2];
- rb_pid_t owner_process;
+
+ /* volatile for signal handler use: */
+ volatile rb_pid_t owner_process;
+ rb_atomic_t writing;
} timer_thread_pipe = {
{-1, -1},
{-1, -1}, /* low priority */
@@ -1280,12 +1287,13 @@ static struct {
/* only use signal-safe system calls here */
static void
-rb_thread_wakeup_timer_thread_fd(int fd)
+rb_thread_wakeup_timer_thread_fd(volatile int *fdp)
{
ssize_t result;
+ int fd = *fdp; /* access fdp exactly once here and do not reread fdp */
/* already opened */
- if (timer_thread_pipe.owner_process == getpid()) {
+ if (fd >= 0 && timer_thread_pipe.owner_process == getpid()) {
const char *buff = "!";
retry:
if ((result = write(fd, buff, 1)) <= 0) {
@@ -1311,13 +1319,18 @@ rb_thread_wakeup_timer_thread_fd(int fd)
void
rb_thread_wakeup_timer_thread(void)
{
- rb_thread_wakeup_timer_thread_fd(timer_thread_pipe.normal[1]);
+ /* must be safe inside sighandler, so no mutex */
+ ATOMIC_INC(timer_thread_pipe.writing);
+ rb_thread_wakeup_timer_thread_fd(&timer_thread_pipe.normal[1]);
+ ATOMIC_DEC(timer_thread_pipe.writing);
}
static void
rb_thread_wakeup_timer_thread_low(void)
{
- rb_thread_wakeup_timer_thread_fd(timer_thread_pipe.low[1]);
+ ATOMIC_INC(timer_thread_pipe.writing);
+ rb_thread_wakeup_timer_thread_fd(&timer_thread_pipe.low[1]);
+ ATOMIC_DEC(timer_thread_pipe.writing);
}
/* VM-dependent API is not available for this function */
@@ -1351,16 +1364,16 @@ consume_communication_pipe(int fd)
}
}
+#define CLOSE_INVALIDATE(expr) close_invalidate(&expr,#expr)
static void
-close_communication_pipe(int pipes[2])
+close_invalidate(volatile int *fdp, const char *msg)
{
- if (close(pipes[0]) < 0) {
- rb_bug_errno("native_stop_timer_thread - close(ttp[0])", errno);
- }
- if (close(pipes[1]) < 0) {
- rb_bug_errno("native_stop_timer_thread - close(ttp[1])", errno);
+ int fd = *fdp; /* access fdp exactly once here and do not reread fdp */
+
+ *fdp = -1;
+ if (close(fd) < 0) {
+ rb_async_bug_errno(msg, errno);
}
- pipes[0] = pipes[1] = -1;
}
static void
@@ -1378,39 +1391,47 @@ set_nonblock(int fd)
rb_sys_fail(0);
}
-static void
+static int
setup_communication_pipe_internal(int pipes[2])
{
int err;
- if (pipes[0] != -1) {
- /* close pipe of parent process */
- close_communication_pipe(pipes);
- }
-
err = rb_cloexec_pipe(pipes);
if (err != 0) {
- rb_bug_errno("setup_communication_pipe: Failed to create communication pipe for timer thread", errno);
+ rb_warn("Failed to create communication pipe for timer thread: %s",
+ strerror(errno));
+ return -1;
}
rb_update_max_fd(pipes[0]);
rb_update_max_fd(pipes[1]);
set_nonblock(pipes[0]);
set_nonblock(pipes[1]);
+ return 0;
}
/* communication pipe with timer thread and signal handler */
-static void
+static int
setup_communication_pipe(void)
{
- if (timer_thread_pipe.owner_process == getpid()) {
- /* already set up. */
- return;
+ VM_ASSERT(timer_thread_pipe.owner_process == 0);
+ VM_ASSERT(timer_thread_pipe.normal[0] == -1);
+ VM_ASSERT(timer_thread_pipe.normal[1] == -1);
+ VM_ASSERT(timer_thread_pipe.low[0] == -1);
+ VM_ASSERT(timer_thread_pipe.low[1] == -1);
+
+ if (setup_communication_pipe_internal(timer_thread_pipe.normal) < 0) {
+ return errno;
+ }
+ if (setup_communication_pipe_internal(timer_thread_pipe.low) < 0) {
+ int e = errno;
+ CLOSE_INVALIDATE(timer_thread_pipe.normal[0]);
+ CLOSE_INVALIDATE(timer_thread_pipe.normal[1]);
+ return e;
}
- setup_communication_pipe_internal(timer_thread_pipe.normal);
- setup_communication_pipe_internal(timer_thread_pipe.low);
/* validate pipe on this process */
timer_thread_pipe.owner_process = getpid();
+ return 0;
}
/**
@@ -1547,7 +1568,10 @@ thread_timer(void *p)
/* wait */
timer_thread_sleep(gvl);
}
-#if !USE_SLEEPY_TIMER_THREAD
+#if USE_SLEEPY_TIMER_THREAD
+ CLOSE_INVALIDATE(timer_thread_pipe.normal[0]);
+ CLOSE_INVALIDATE(timer_thread_pipe.low[0]);
+#else
native_mutex_unlock(&timer_thread_lock);
native_cond_destroy(&timer_thread_cond);
native_mutex_destroy(&timer_thread_lock);
@@ -1567,8 +1591,9 @@ rb_thread_create_timer_thread(void)
err = pthread_attr_init(&attr);
if (err != 0) {
- fprintf(stderr, "[FATAL] Failed to initialize pthread attr: %s\n", strerror(err));
- exit(EXIT_FAILURE);
+ rb_warn("pthread_attr_init failed for timer: %s, scheduling broken",
+ strerror(err));
+ return;
}
# ifdef PTHREAD_STACK_MIN
{
@@ -1586,7 +1611,12 @@ rb_thread_create_timer_thread(void)
#endif
#if USE_SLEEPY_TIMER_THREAD
- setup_communication_pipe();
+ err = setup_communication_pipe();
+ if (err != 0) {
+ rb_warn("pipe creation failed for timer: %s, scheduling broken",
+ strerror(err));
+ return;
+ }
#endif /* USE_SLEEPY_TIMER_THREAD */
/* create timer thread */
@@ -1599,8 +1629,13 @@ rb_thread_create_timer_thread(void)
err = pthread_create(&timer_thread.id, NULL, thread_timer, &GET_VM()->gvl);
#endif
if (err != 0) {
- fprintf(stderr, "[FATAL] Failed to create timer thread: %s\n", strerror(err));
- exit(EXIT_FAILURE);
+ rb_warn("pthread_create failed for timer: %s, scheduling broken",
+ strerror(err));
+ CLOSE_INVALIDATE(timer_thread_pipe.normal[0]);
+ CLOSE_INVALIDATE(timer_thread_pipe.normal[1]);
+ CLOSE_INVALIDATE(timer_thread_pipe.low[0]);
+ CLOSE_INVALIDATE(timer_thread_pipe.low[1]);
+ return;
}
timer_thread.created = 1;
#ifdef HAVE_PTHREAD_ATTR_INIT
@@ -1610,30 +1645,38 @@ rb_thread_create_timer_thread(void)
}
static int
-native_stop_timer_thread(int close_anyway)
+native_stop_timer_thread(void)
{
int stopped;
stopped = --system_working <= 0;
if (TT_DEBUG) fprintf(stderr, "stop timer thread\n");
if (stopped) {
- /* join */
- rb_thread_wakeup_timer_thread();
+ /* prevent wakeups from signal handler ASAP */
+ timer_thread_pipe.owner_process = 0;
+
+ /*
+ * however, the above was not enough: the FD may already be
+ * captured and in the middle of a write while we are running,
+ * so wait for that to finish:
+ */
+ while (ATOMIC_CAS(timer_thread_pipe.writing, 0, 0)) {
+ native_thread_yield();
+ }
+
+ /* stop writing ends of pipes so timer thread notices EOF */
+ CLOSE_INVALIDATE(timer_thread_pipe.normal[1]);
+ CLOSE_INVALIDATE(timer_thread_pipe.low[1]);
+
+ /* timer thread will stop looping when system_working <= 0: */
native_thread_join(timer_thread.id);
+
+ /* timer thread will close the read end on exit: */
+ VM_ASSERT(timer_thread_pipe.normal[0] == -1);
+ VM_ASSERT(timer_thread_pipe.low[0] == -1);
+
if (TT_DEBUG) fprintf(stderr, "joined timer thread\n");
timer_thread.created = 0;
-
- /* close communication pipe */
- if (close_anyway) {
- /* TODO: Uninstall all signal handlers or mask all signals.
- * This pass is cleaning phase (terminate ruby process).
- * To avoid such race, we skip to close communication
- * pipe. OS will close it at process termination.
- * It may not good practice, but pragmatic.
- * We remain it is TODO.
- */
- /* close_communication_pipe(); */
- }
}
return stopped;
}
@@ -1707,29 +1750,6 @@ rb_reserved_fd_p(int fd)
#endif
}
-int
-rb_divert_reserved_fd(int fd)
-{
-#if USE_SLEEPY_TIMER_THREAD
- int *ptr;
- int newfd;
-
- if ((fd == *(ptr = &(timer_thread_pipe.normal[0])) ||
- fd == *(ptr = &(timer_thread_pipe.normal[1])) ||
- fd == *(ptr = &(timer_thread_pipe.low[0])) ||
- fd == *(ptr = &(timer_thread_pipe.low[1]))) &&
- timer_thread_pipe.owner_process == getpid()) { /* async-signal-safe */
- newfd = rb_cloexec_dup(fd); /* async-signal-safe if no error */
- if (newfd == -1) return -1;
- rb_update_max_fd(newfd); /* async-signal-safe if no error */
- /* set_nonblock(newfd); */ /* async-signal-safe if no error */
- *ptr = newfd;
- rb_thread_wakeup_timer_thread_low(); /* async-signal-safe? */
- }
-#endif
- return 0;
-}
-
rb_nativethread_id_t
rb_nativethread_self(void)
{
diff --git a/thread_win32.c b/thread_win32.c
index 2dd377d..2dfbf06 100644
--- a/thread_win32.c
+++ b/thread_win32.c
@@ -730,7 +730,7 @@ rb_thread_create_timer_thread(void)
}
static int
-native_stop_timer_thread(int close_anyway)
+native_stop_timer_thread(void)
{
int stopped = --system_working <= 0;
if (stopped) {
@@ -788,12 +788,6 @@ rb_reserved_fd_p(int fd)
return 0;
}
-int
-rb_divert_reserved_fd(int fd)
-{
- return 0;
-}
-
rb_nativethread_id_t
rb_nativethread_self(void)
{
diff --git a/vm_core.h b/vm_core.h
index a032daa..cad0e09 100644
--- a/vm_core.h
+++ b/vm_core.h
@@ -979,7 +979,7 @@ VALUE rb_vm_call(rb_thread_t *th, VALUE recv, VALUE id, int argc,
const VALUE *argv, const rb_callable_method_entry_t *me);
void rb_thread_start_timer_thread(void);
-void rb_thread_stop_timer_thread(int);
+void rb_thread_stop_timer_thread(void);
void rb_thread_reset_timer_thread(void);
void rb_thread_wakeup_timer_thread(void);