ruby-changes:51643
From: normal <ko1@a...>
Date: Thu, 5 Jul 2018 12:02:45 +0900 (JST)
Subject: [ruby-changes:51643] normal:r63855 (trunk): unrevert r63852 but keep SIGCHLD path disabled for win32
normal 2018-07-05 12:02:33 +0900 (Thu, 05 Jul 2018) New Revision: 63855 https://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=revision&revision=63855 Log: unrevert r63852 but keep SIGCHLD path disabled for win32 Reading win32/win32.c waitpid implementation, maybe waitpid(-1, ...) on that platform will never conflict with mjit use of waitpid. In any case, I've added WAITPID_USE_SIGCHLD macro to vm_core.h so it can be easy for Linux/BSD users to test (hopefully!) win32-compatible code. Modified files: trunk/configure.ac trunk/ext/pty/pty.c trunk/internal.h trunk/mjit.c trunk/process.c trunk/signal.c trunk/spec/ruby/core/process/wait2_spec.rb trunk/spec/ruby/core/process/wait_spec.rb trunk/spec/ruby/core/process/waitall_spec.rb trunk/test/ruby/test_optimization.rb trunk/test/ruby/test_process.rb trunk/test/ruby/test_rubyoptions.rb trunk/thread.c trunk/thread_pthread.c trunk/vm_core.h trunk/win32/Makefile.sub Index: thread_pthread.c =================================================================== --- thread_pthread.c (revision 63854) +++ thread_pthread.c (revision 63855) @@ -1369,7 +1369,7 @@ setup_communication_pipe(void) https://github.com/ruby/ruby/blob/trunk/thread_pthread.c#L1369 * @pre the calling context is in the timer thread. */ static inline void -timer_thread_sleep(rb_global_vm_lock_t* gvl) +timer_thread_sleep(rb_vm_t *vm) { int result; int need_polling; @@ -1382,7 +1382,15 @@ timer_thread_sleep(rb_global_vm_lock_t* https://github.com/ruby/ruby/blob/trunk/thread_pthread.c#L1382 need_polling = !ubf_threads_empty(); - if (gvl->waiting > 0 || need_polling) { + if (SIGCHLD_LOSSY && !need_polling) { + rb_native_mutex_lock(&vm->waitpid_lock); + if (!list_empty(&vm->waiting_pids) || !list_empty(&vm->waiting_grps)) { + need_polling = 1; + } + rb_native_mutex_unlock(&vm->waitpid_lock); + } + + if (vm->gvl.waiting > 0 || need_polling) { /* polling (TIME_QUANTUM_USEC usec) */ result = poll(pollfds, 1, TIME_QUANTUM_USEC/1000); } @@ -1421,7 +1429,7 @@ static rb_nativethread_lock_t timer_thre https://github.com/ruby/ruby/blob/trunk/thread_pthread.c#L1429 static rb_nativethread_cond_t timer_thread_cond; static inline void -timer_thread_sleep(rb_global_vm_lock_t* unused) +timer_thread_sleep(rb_vm_t *unused) { struct timespec ts; ts.tv_sec = 0; @@ -1485,7 +1493,14 @@ native_set_another_thread_name(rb_native https://github.com/ruby/ruby/blob/trunk/thread_pthread.c#L1493 static void * thread_timer(void *p) { - rb_global_vm_lock_t *gvl = (rb_global_vm_lock_t *)p; + rb_vm_t *vm = p; +#ifdef HAVE_PTHREAD_SIGMASK /* mainly to enable SIGCHLD */ + { + sigset_t mask; + sigemptyset(&mask); + pthread_sigmask(SIG_SETMASK, &mask, NULL); + } +#endif if (TT_DEBUG) WRITE_CONST(2, "start timer thread\n"); @@ -1507,7 +1522,7 @@ thread_timer(void *p) https://github.com/ruby/ruby/blob/trunk/thread_pthread.c#L1522 if (TT_DEBUG) WRITE_CONST(2, "tick\n"); /* wait */ - timer_thread_sleep(gvl); + timer_thread_sleep(vm); } #if USE_SLEEPY_TIMER_THREAD CLOSE_INVALIDATE(normal[0]); @@ -1579,7 +1594,7 @@ rb_thread_create_timer_thread(void) https://github.com/ruby/ruby/blob/trunk/thread_pthread.c#L1594 if (timer_thread.created) { rb_bug("rb_thread_create_timer_thread: Timer thread was already created\n"); } - err = pthread_create(&timer_thread.id, &attr, thread_timer, &vm->gvl); + err = pthread_create(&timer_thread.id, &attr, thread_timer, vm); pthread_attr_destroy(&attr); if (err == EINVAL) { @@ -1590,7 +1605,7 @@ rb_thread_create_timer_thread(void) https://github.com/ruby/ruby/blob/trunk/thread_pthread.c#L1605 * default stack size is enough for them: */ stack_size = 0; - err = pthread_create(&timer_thread.id, NULL, thread_timer, &vm->gvl); + err = pthread_create(&timer_thread.id, NULL, thread_timer, vm); } if (err != 0) { rb_warn("pthread_create failed for timer: %s, scheduling broken", @@ -1771,4 +1786,22 @@ rb_thread_create_mjit_thread(void (*chil https://github.com/ruby/ruby/blob/trunk/thread_pthread.c#L1786 return ret; } +#define USE_NATIVE_SLEEP_COND (1) + +#if USE_NATIVE_SLEEP_COND +rb_nativethread_cond_t * +rb_sleep_cond_get(const rb_execution_context_t *ec) +{ + rb_thread_t *th = rb_ec_thread_ptr(ec); + + return &th->native_thread_data.sleep_cond; +} + +void +rb_sleep_cond_put(rb_nativethread_cond_t *cond) +{ + /* no-op */ +} +#endif /* USE_NATIVE_SLEEP_COND */ + #endif /* THREAD_SYSTEM_DEPENDENT_IMPLEMENTATION */ Index: mjit.c =================================================================== --- mjit.c (revision 63854) +++ mjit.c (revision 63855) @@ -80,6 +80,7 @@ https://github.com/ruby/ruby/blob/trunk/mjit.c#L80 #include "constant.h" #include "id_table.h" #include "ruby_assert.h" +#include "ruby/thread.h" #include "ruby/util.h" #include "ruby/version.h" @@ -118,6 +119,10 @@ extern void rb_native_cond_wait(rb_nativ https://github.com/ruby/ruby/blob/trunk/mjit.c#L119 extern int rb_thread_create_mjit_thread(void (*child_hook)(void), void (*worker_func)(void)); +/* process.c */ +rb_pid_t ruby_waitpid_locked(rb_vm_t *, rb_pid_t, int *status, int options, + rb_nativethread_cond_t *cond); + #define RB_CONDATTR_CLOCK_MONOTONIC 1 #ifdef _WIN32 @@ -263,7 +268,7 @@ real_ms_time(void) https://github.com/ruby/ruby/blob/trunk/mjit.c#L268 static int sprint_uniq_filename(char *str, size_t size, unsigned long id, const char *prefix, const char *suffix) { - return snprintf(str, size, "%s/%sp%luu%lu%s", tmp_dir, prefix, (unsigned long) getpid(), id, suffix); + return snprintf(str, size, "%s/%sp%"PRI_PIDT_PREFIX"uu%lu%s", tmp_dir, prefix, getpid(), id, suffix); } /* Return an unique file name in /tmp with PREFIX and SUFFIX and @@ -401,22 +406,41 @@ start_process(const char *path, char *co https://github.com/ruby/ruby/blob/trunk/mjit.c#L406 static int exec_process(const char *path, char *const argv[]) { - int stat, exit_code; + int stat, exit_code = -2; pid_t pid; + rb_vm_t *vm = WAITPID_USE_SIGCHLD ? GET_VM() : 0; + rb_nativethread_cond_t cond; - pid = start_process(path, argv); - if (pid <= 0) - return -2; + if (vm) { + rb_native_cond_initialize(&cond); + rb_native_mutex_lock(&vm->waitpid_lock); + } - for (;;) { - waitpid(pid, &stat, 0); - if (WIFEXITED(stat)) { - exit_code = WEXITSTATUS(stat); - break; - } else if (WIFSIGNALED(stat)) { - exit_code = -1; + pid = start_process(path, argv); + for (;pid > 0;) { + pid_t r = vm ? ruby_waitpid_locked(vm, pid, &stat, 0, &cond) + : waitpid(pid, &stat, 0); + if (r == -1) { + if (errno == EINTR) continue; + fprintf(stderr, "[%"PRI_PIDT_PREFIX"d] waitpid(%"PRI_PIDT_PREFIX"d): %s (SIGCHLD=%d,%u)\n", + getpid(), pid, strerror(errno), + RUBY_SIGCHLD, SIGCHLD_LOSSY); break; } + else if (r == pid) { + if (WIFEXITED(stat)) { + exit_code = WEXITSTATUS(stat); + break; + } else if (WIFSIGNALED(stat)) { + exit_code = -1; + break; + } + } + } + + if (vm) { + rb_native_mutex_unlock(&vm->waitpid_lock); + rb_native_cond_destroy(&cond); } return exit_code; } @@ -1491,12 +1515,15 @@ mjit_init(struct mjit_options *opts) https://github.com/ruby/ruby/blob/trunk/mjit.c#L1515 static void stop_worker(void) { + rb_execution_context_t *ec = GET_EC(); + stop_worker_p = TRUE; while (!worker_stopped) { verbose(3, "Sending cancel signal to worker"); CRITICAL_SECTION_START(3, "in stop_worker"); rb_native_cond_broadcast(&mjit_worker_wakeup); CRITICAL_SECTION_FINISH(3, "in stop_worker"); + RUBY_VM_CHECK_INTS(ec); } } Index: test/ruby/test_process.rb =================================================================== --- test/ruby/test_process.rb (revision 63854) +++ test/ruby/test_process.rb (revision 63855) @@ -1426,7 +1426,6 @@ class TestProcess < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_process.rb#L1426 end def test_wait_without_arg - skip "[Bug #14867]" if RubyVM::MJIT.enabled? with_tmpchdir do write_file("foo", "sleep 0.1") pid = spawn(RUBY, "foo") @@ -1435,7 +1434,6 @@ class TestProcess < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_process.rb#L1434 end def test_wait2 - skip "[Bug #14867]" if RubyVM::MJIT.enabled? with_tmpchdir do write_file("foo", "sleep 0.1") pid = spawn(RUBY, "foo") @@ -1444,7 +1442,6 @@ class TestProcess < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_process.rb#L1442 end def test_waitall - skip "[Bug #14867]" if RubyVM::MJIT.enabled? with_tmpchdir do write_file("foo", "sleep 0.1") ps = (0...3).map { spawn(RUBY, "foo") }.sort @@ -1459,7 +1456,9 @@ class TestProcess < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_process.rb#L1456 def test_wait_exception bug11340 = '[ruby-dev:49176] [Bug #11340]' t0 = t1 = nil - IO.popen([RUBY, '-e', 'puts;STDOUT.flush;Thread.start{gets;exit};sleep(3)'], 'r+') do |f| + sec = 3 + code = "puts;STDOUT.flush;Thread.start{gets;exit};sleep(#{sec})" + IO.popen([RUBY, '-e', code], 'r+') do |f| pid = f.pid f.gets t0 = Time.now @@ -1473,10 +1472,11 @@ class TestProcess < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_process.rb#L1472 th.kill.join end t1 = Time.now + diff = t1 - t0 + assert_operator(diff, :<, sec, + ->{"#{bug11340}: #{diff} seconds to interrupt Process.wait"}) f.puts end - assert_operator(t1 - t0, :<, 3, - ->{"#{bug11340}: #{t1-t0} seconds to interrupt Process.wait"}) end def test_abort Index: test/ruby/test_rubyoptions.rb =================================================================== --- test/ruby/test_rubyoptions.rb (revision 63854) +++ test/ruby/test_rubyoptions.rb (revision 63855) @@ -1,6 +1,7 @@ https://github.com/ruby/ruby/blob/trunk/test/ruby/test_rubyoptions.rb#L1 # -*- coding: us-ascii -*- require 'test/unit' +require 'timeout' require 'tmpdir' require 'tempfile' require_relative '../lib/jit_support' @@ -590,14 +591,18 @@ class TestRubyOptions < Test::Unit::Test https://github.com/ruby/ruby/blob/trunk/test/ruby/test_rubyoptions.rb#L591 pid = spawn(EnvUtil.rubybin, "test-script") ps = nil + now = Process.clock_gettime(Process::CLOCK_MONOTONIC) + stop = now + 30 begin sleep 0.1 ps = `#{PSCMD.join(' ')} #{pid}` break if /hello world/ =~ ps - end until Process.wait(pid, Process::WNOHANG) + now = Process.clock_gettime(Process::CLOCK_MONOTONIC) + end until Process.wait(pid, Process::WNOHANG) || now > stop assert_match(/hello world/, ps) + assert_operator now, :<, stop Process.kill :KILL, pid - Process.wait(pid) + Timeout.timeout(5) { Process.wait(pid) } end end @@ -616,14 +621,18 @@ class TestRubyOptions < Test::Unit::Test https://github.com/ruby/ruby/blob/trunk/test/ruby/test_rubyoptions.rb#L621 pid = spawn(EnvUtil.rubybin, "test-script") ps = nil + now = Process.clock_gettime(Process::CLOCK_MONOTONIC) + stop = now + 30 begin sleep 0.1 ps = `#{PSCMD.join(' ')} #{pid}` break if /hello world/ =~ ps - end until Process.wait(pid, Process::WNOHANG) + now = Process.clock_gettime(Process::CLOCK_MONOTONIC) + end until Process.wait(pid, Process::WNOHANG) || now > stop assert_match(/hello world/, ps) + assert_operator now, :<, stop Process.kill :KILL, pid - Process.wait(pid) + Timeout.timeout(5) { Process.wait(pid) } end end Index: test/ruby/test_optimization.rb =================================================================== --- test/ruby/test_optimization.rb (revision 63854) +++ test/ruby/test_optimization.rb (revision 63855) @@ -707,7 +707,7 @@ class TestRubyOptimization < Test::Unit: https://github.com/ruby/ruby/blob/trunk/test/ruby/test_optimization.rb#L707 end def test_clear_unreachable_keyword_args - assert_separately [], <<-END, timeout: 20 + assert_separately [], <<-END, timeout: 30 script = <<-EOS if true else Index: vm_core.h =================================================================== --- vm_core.h (revision 63854) +++ vm_core.h (revision 63855) @@ -92,6 +92,24 @@ https://github.com/ruby/ruby/blob/trunk/vm_core.h#L92 #define RUBY_NSIG NSIG +#if defined(SIGCLD) +# define RUBY_SIGCHLD (SIGCLD) +#elif defined(SIGCHLD) +# define RUBY_SIGCHLD (SIGCHLD) +#else +# define RUBY_SIGCHLD (0) +#endif + +/* platforms with broken or non-existent SIGCHLD work by polling */ +#if defined(__APPLE__) +# define SIGCHLD_LOSSY (1) +#else +# define SIGCHLD_LOSSY (0) +#endif + +/* define to 0 to test old code path */ +#define WAITPID_USE_SIGCHLD (RUBY_SIGCHLD || SIGCHLD_LOSSY) + #ifdef HAVE_STDARG_PROTOTYPES #include <stdarg.h> #define va_init_list(a,b) va_start((a),(b)) @@ -553,6 +571,9 @@ typedef struct rb_vm_struct { https://github.com/ruby/ruby/blob/trunk/vm_core.h#L571 #endif rb_serial_t fork_gen; + rb_nativethread_lock_t waitpid_lock; + struct list_head waiting_pids; /* PID > 0: <=> struct waitpid_state */ + struct list_head waiting_grps; /* PID <= 0: <=> struct waitpid_state */ struct list_head waiting_fds; /* <=> struct waiting_fd */ struct list_head living_threads; VALUE thgroup_default; @@ -1561,6 +1582,8 @@ static inline void https://github.com/ruby/ruby/blob/trunk/vm_core.h#L1582 rb_vm_living_threads_init(rb_vm_t *vm) { list_head_init(&vm->waiting_fds); + list_head_init(&vm->waiting_pids); + list_head_init(&vm->waiting_grps); list_head_init(&vm->living_threads); vm->living_thread_num = 0; } Index: thread.c =================================================================== --- thread.c (revision 63854) +++ thread.c (revision 63855) @@ -413,6 +413,10 @@ rb_vm_gvl_destroy(rb_vm_t *vm) https://github.com/ruby/ruby/blob/trunk/thread.c#L413 gvl_release(vm); gvl_destroy(vm); rb_native_mutex_destroy(&vm->thread_destruct_lock); + if (0) { + /* may be held by running threads */ + rb_native_mutex_destroy(&vm->waitpid_lock); + } } void @@ -4131,6 +4135,9 @@ rb_gc_set_stack_end(VALUE **stack_end_p) https://github.com/ruby/ruby/blob/trunk/thread.c#L4135 #endif +/* signal.c */ +void ruby_sigchld_handler(rb_vm_t *); + /* * */ @@ -4163,6 +4170,7 @@ timer_thread_function(void *arg) https://github.com/ruby/ruby/blob/trunk/thread.c#L4170 rb_native_mutex_unlock(&vm->thread_destruct_lock); /* check signal */ + ruby_sigchld_handler(vm); rb_threadptr_check_signal(vm->main_thread); #if 0 @@ -4247,6 +4255,9 @@ rb_thread_atfork_internal(rb_thread_t *t https://github.com/ruby/ruby/blob/trunk/thread.c#L4255 } rb_vm_living_threads_init(vm); rb_vm_living_threads_insert(vm, th); + + /* may be held by MJIT threads in parent */ + rb_native_mutex_initialize(&vm->waitpid_lock); vm->fork_gen++; vm->sleeper = 0; @@ -4999,6 +5010,7 @@ Init_Thread(void) https://github.com/ruby/ruby/blob/trunk/thread.c#L5010 gvl_init(th->vm); gvl_acquire(th->vm, th); rb_native_mutex_initialize(&th->vm->thread_destruct_lock); + rb_native_mutex_initialize(&th->vm->waitpid_lock); rb_native_mutex_initialize(&th->interrupt_lock); th->pending_interrupt_queue = rb_ary_tmp_new(0); @@ -5302,3 +5314,25 @@ rb_uninterruptible(VALUE (*b_proc)(ANYAR https://github.com/ruby/ruby/blob/trunk/thread.c#L5314 return rb_ensure(b_proc, data, rb_ary_pop, cur_th->pending_interrupt_mask_stack); } + +#ifndef USE_NATIVE_SLEEP_COND +# define USE_NATIVE_SLEEP_COND (0) +#endif + +#if !USE_NATIVE_SLEEP_COND +rb_nativethread_cond_t * +rb_sleep_cond_get(const rb_execution_context_t *ec) +{ + rb_nativethread_cond_t *cond = ALLOC(rb_nativethread_cond_t); + rb_native_cond_initialize(cond); + + return cond; +} + +void +rb_sleep_cond_put(rb_nativethread_cond_t *cond) +{ + rb_native_cond_destroy(cond); + xfree(cond); +} +#endif /* !USE_NATIVE_SLEEP_COND */ Index: configure.ac =================================================================== --- configure.ac (revision 63854) +++ configure.ac (revision 63855) @@ -766,6 +766,7 @@ AS_CASE(["$target_os"], https://github.com/ruby/ruby/blob/trunk/configure.ac#L766 AS_IF([test $gcc_major -lt 4 -o \( $gcc_major -eq 4 -a $gcc_minor -lt 3 \)], [ ac_cv_func___builtin_setjmp=no ]) + with_setjmp_type=sigsetjmp # to hijack SIGCHLD handler AC_CACHE_CHECK(for broken crypt with 8bit chars, rb_cv_broken_crypt, [AC_TRY_RUN([ #include <stdio.h> @@ -1782,6 +1783,7 @@ AC_CHECK_FUNCS(getsid) https://github.com/ruby/ruby/blob/trunk/configure.ac#L1783 AC_CHECK_FUNCS(gettimeofday) # for making ac_cv_func_gettimeofday AC_CHECK_FUNCS(getuidx) AC_CHECK_FUNCS(gmtime_r) +AC_CHECK_FUNCS(grantpt) AC_CHECK_FUNCS(initgroups) AC_CHECK_FUNCS(ioctl) AC_CHECK_FUNCS(isfinite) Index: process.c =================================================================== --- process.c (revision 63854) +++ process.c (revision 63855) @@ -885,12 +885,6 @@ pst_wcoredump(VALUE st) https://github.com/ruby/ruby/blob/trunk/process.c#L885 #endif } -struct waitpid_arg { - rb_pid_t pid; - int flags; - int *st; -}; - static rb_pid_t do_waitpid(rb_pid_t pid, int *st, int flags) { @@ -903,45 +897,263 @@ do_waitpid(rb_pid_t pid, int *st, int fl https://github.com/ruby/ruby/blob/trunk/process.c#L897 #endif } +struct waitpid_state { + struct list_node wnode; + rb_execution_context_t *ec; + rb_nativethread_cond_t *cond; + rb_pid_t ret; + rb_pid_t pid; + int status; + int options; + int errnum; +}; + +void rb_native_mutex_lock(rb_nativethread_lock_t *); +void rb_native_mutex_unlock(rb_nativethread_lock_t *); +void rb_native_cond_signal(rb_nativethread_cond_t *); +void rb_native_cond_wait(rb_nativethread_cond_t *, rb_nativethread_lock_t *); +rb_nativethread_cond_t *rb_sleep_cond_get(const rb_execution_context_t *); +void rb_sleep_cond_put(rb_nativethread_cond_t *); + +static void +waitpid_notify(struct waitpid_state *w, rb_pid_t ret) +{ + w->ret = ret; + list_del_init(&w->wnode); + rb_native_cond_signal(w->cond); +} + +#ifdef _WIN32 /* for spawnvp result from mjit.c */ +# define waitpid_sys(pid,status,options) \ + (WaitForSingleObject((HANDLE)(pid), 0),\ + GetExitCodeProcess((HANDLE)(pid), (LPDWORD)(status))) +#else +# define waitpid_sys(pid,status,options) do_waitpid((pid),(status),(options)) +#endif + +/* called by timer thread */ +static void +waitpid_each(struct list_head *head) +{ + struct waitpid_state *w = 0, *next; + + list_for_each_safe(head, w, next, wnode) { + rb_pid_t ret; + + if (w->ec) + ret = do_waitpid(w->pid, &w->status, w->options | WNOHANG); + else + ret = waitpid_sys(w->pid, &w->status, w->options | WNOHANG); + + if (!ret) continue; + if (ret == -1) w->errnum = errno; + + if (w->ec) { /* rb_waitpid */ + rb_thread_t *th = rb_ec_thread_ptr(w->ec); + + rb_native_mutex_lock(&th->interrupt_lock); + waitpid_notify(w, ret); + rb_native_mutex_unlock(&th->interrupt_lock); + } + else { /* ruby_waitpid_locked */ + waitpid_notify(w, ret); + } + } +} + +void +ruby_waitpid_all(rb_vm_t *vm) +{ + rb_native_mutex_lock(&vm->waitpid_lock); + waitpid_each(&vm->waiting_pids); + if (list_empty(&vm->waiting_pids)) { + waitpid_each(&vm->waiting_grps); + } + rb_native_mutex_unlock(&vm->waitpid_lock); +} + +static void +waitpid_state_init(struct waitpid_state *w, rb_pid_t pid, int options) +{ + w->ret = 0; + w->pid = pid; + w->options = options; +} + +/* + * must be called with vm->waitpid_lock held, this is not interruptible + */ +rb_pid_t +ruby_waitpid_locked(rb_vm_t *vm, rb_pid_t pid, int *status, int options, + rb_nativethread_cond_t *cond) +{ + struct waitpid_state w; + + assert(!ruby_thread_has_gvl_p() && "must not have GVL"); + + waitpid_state_init(&w, pid, options); + if (w.pid > 0 || list_empty(&vm->waiting_pids)) + w.ret = do_waitpid(w.pid, &w.status (... truncated) -- ML: ruby-changes@q... Info: http://www.atdot.net/~ko1/quickml/