From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Andreas Politz Newsgroups: gmane.emacs.devel Subject: condition-wait considered harmful Date: Wed, 30 Oct 2019 22:26:21 +0100 Message-ID: <87eeyudiwy.fsf@hochschule-trier.de> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="48462"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Oct 30 22:29:16 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1iPvWm-000CVR-4J for ged-emacs-devel@m.gmane.org; Wed, 30 Oct 2019 22:29:16 +0100 Original-Received: from localhost ([::1]:44526 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iPvWk-0006v8-CQ for ged-emacs-devel@m.gmane.org; Wed, 30 Oct 2019 17:29:14 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:33092) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iPvU8-0006qq-Oh for emacs-devel@gnu.org; Wed, 30 Oct 2019 17:26:34 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iPvU6-0001k6-PG for emacs-devel@gnu.org; Wed, 30 Oct 2019 17:26:32 -0400 Original-Received: from gateway-a.fh-trier.de ([143.93.54.181]:47756) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1iPvU6-0001bg-40 for emacs-devel@gnu.org; Wed, 30 Oct 2019 17:26:30 -0400 X-Virus-Scanned: by Amavisd-new + Sophos + ClamAV [Rechenzentrum Hochschule Trier (RZ/HT)] Original-Received: from localhost (x4d0ca2a2.dyn.telefonica.de [77.12.162.162]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) (Authenticated sender: politza) by gateway-a.fh-trier.de (Postfix) with ESMTPSA id B3DC0190F9CE for ; Wed, 30 Oct 2019 22:26:24 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha1; c=simple/simple; d=hochschule-trier.de; s=default; t=1572470784; bh=C18wWnc5mgYrwDJklO27qUWTRqM=; h=From:To:Subject:Date:Message-ID:MIME-Version:Content-Type; b=bqAXyZzGI/spJ/KL90CofbdMtODOFkhgWc7ctxjeMVtxuK3bI5XMwQgQ2c0j2xqI4 GMlQdQPM7eFxYaNR+y/NOAqK3+9M5AqhF1qFlQg/7HoFKaFQtUo7s2B/zs2nE9qeAN EuElSkhTZkwZLLHzfhlmMkLw44u6pZvB/RZMximU= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x [fuzzy] X-Received-From: 143.93.54.181 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:241648 Archived-At: --=-=-= Content-Type: text/plain Please consider adding a mandatory timeout (see attached patch) when calling condition-wait in the main-thread. As it currently is, any such call may make Emacs freeze irrecoverably. I hope no argument is needed, why this is a bad thing. Andreas --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=cond_timedwait.patch Content-Description: condition_timedwait.patch diff --git a/src/systhread.c b/src/systhread.c index 6f4de536fb..77603fcbcd 100644 --- a/src/systhread.c +++ b/src/systhread.c @@ -51,7 +51,7 @@ sys_cond_init (sys_cond_t *c) } void -sys_cond_wait (sys_cond_t *c, sys_mutex_t *m) +sys_cond_wait (sys_cond_t *c, sys_mutex_t *m, int max_seconds) { } @@ -158,10 +158,20 @@ sys_cond_init (sys_cond_t *cond) } void -sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex) +sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex, int max_seconds) { - int error = pthread_cond_wait (cond, mutex); - eassert (error == 0); + int error; + + if (max_seconds < 0) + error = pthread_cond_wait (cond, mutex); + else + { + struct timespec timeout; + clock_gettime(CLOCK_REALTIME, &timeout); + timeout.tv_sec += max_seconds; + error = pthread_cond_timedwait (cond, mutex, &timeout); + } + eassert (error == 0 || error != ETIMEDOUT); } void @@ -294,9 +304,10 @@ sys_cond_init (sys_cond_t *cond) } void -sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex) +sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex, int max_seconds) { DWORD wait_result; + DWORD max_millis = max_seconds < 0 ? INFINITE : 1000 * max_seconds; bool last_thread_waiting; if (!cond->initialized) @@ -310,7 +321,7 @@ sys_cond_wait (sys_cond_t *cond, sys_mutex_t *mutex) /* Release the mutex and wait for either the signal or the broadcast event. */ LeaveCriticalSection ((LPCRITICAL_SECTION)mutex); - wait_result = WaitForMultipleObjects (2, cond->events, FALSE, INFINITE); + wait_result = WaitForMultipleObjects (2, cond->events, FALSE, max_millis); /* Decrement the wait count and see if we are the last thread waiting on the condition variable. */ diff --git a/src/systhread.h b/src/systhread.h index 8070bcde75..7a1ca615d7 100644 --- a/src/systhread.h +++ b/src/systhread.h @@ -32,6 +32,7 @@ along with GNU Emacs. If not, see . */ #ifdef HAVE_PTHREAD #include +#include /* A system mutex is just a pthread mutex. This is only used for the GIL. */ @@ -102,7 +103,7 @@ extern void sys_mutex_lock (sys_mutex_t *); extern void sys_mutex_unlock (sys_mutex_t *); extern void sys_cond_init (sys_cond_t *); -extern void sys_cond_wait (sys_cond_t *, sys_mutex_t *); +extern void sys_cond_wait (sys_cond_t *, sys_mutex_t *, int); extern void sys_cond_signal (sys_cond_t *); extern void sys_cond_broadcast (sys_cond_t *); extern void sys_cond_destroy (sys_cond_t *); diff --git a/src/thread.c b/src/thread.c index e2deadd7a8..92fd6188ec 100644 --- a/src/thread.c +++ b/src/thread.c @@ -193,7 +193,7 @@ lisp_mutex_lock_for_thread (lisp_mutex_t *mutex, struct thread_state *locker, self->wait_condvar = &mutex->condition; while (mutex->owner != NULL && (new_count != 0 || NILP (self->error_symbol))) - sys_cond_wait (&mutex->condition, &global_lock); + sys_cond_wait (&mutex->condition, &global_lock, -1); self->wait_condvar = NULL; if (new_count == 0 && !NILP (self->error_symbol)) @@ -404,10 +404,18 @@ informational only. */) return result; } +struct wait_args +{ + struct Lisp_CondVar *cvar; + int max_seconds; +}; + static void -condition_wait_callback (void *arg) +condition_wait_callback (void *wait_arg) { - struct Lisp_CondVar *cvar = arg; + struct wait_args *arg = wait_arg; + struct Lisp_CondVar *cvar = arg->cvar; + int max_seconds = arg->max_seconds; struct Lisp_Mutex *mutex = XMUTEX (cvar->mutex); struct thread_state *self = current_thread; unsigned int saved_count; @@ -421,7 +429,7 @@ condition_wait_callback (void *arg) { self->wait_condvar = &cvar->cond; /* This call could switch to another thread. */ - sys_cond_wait (&cvar->cond, &global_lock); + sys_cond_wait (&cvar->cond, &global_lock, max_seconds); self->wait_condvar = NULL; } self->event_object = Qnil; @@ -437,9 +445,26 @@ condition_wait_callback (void *arg) post_acquire_global_lock (self); } -DEFUN ("condition-wait", Fcondition_wait, Scondition_wait, 1, 1, 0, +#define MAIN_THREAD_CONDITION_WAIT_MAX 16 + +static int +condition_wait_get_timeout (Lisp_Object specified) +{ + if (NILP (specified)) + XSETINT(specified, -1); + else + CHECK_INTEGER (specified); + + if (main_thread_p (current_thread)) + return max (MAIN_THREAD_CONDITION_WAIT_MAX, XFIXNUM (specified)); + + return XFIXNUM (specified); +} + +DEFUN ("condition-wait", Fcondition_wait, Scondition_wait, 1, 2, 0, doc: /* Wait for the condition variable COND to be notified. -COND is the condition variable to wait on. +COND is the condition variable to wait on. Wait at most SECONDS +seconds. If SECONDS is negative, wait without limit. The mutex associated with COND must be held when this is called. It is an error if it is not held. @@ -448,10 +473,11 @@ This releases the mutex and waits for COND to be notified or for this thread to be signaled with `thread-signal'. When `condition-wait' returns, COND's mutex will again be locked by this thread. */) - (Lisp_Object cond) + (Lisp_Object cond, Lisp_Object seconds) { struct Lisp_CondVar *cvar; struct Lisp_Mutex *mutex; + struct wait_args args; CHECK_CONDVAR (cond); cvar = XCONDVAR (cond); @@ -460,7 +486,9 @@ this thread. */) if (!lisp_mutex_owned_p (&mutex->mutex)) error ("Condition variable's mutex is not held by current thread"); - flush_stack_call_func (condition_wait_callback, cvar); + args.cvar = cvar; + args.max_seconds = condition_wait_get_timeout (seconds); + flush_stack_call_func (condition_wait_callback, &args); return Qnil; } @@ -961,7 +989,7 @@ thread_join_callback (void *arg) self->event_object = thread; self->wait_condvar = &tstate->thread_condvar; while (thread_live_p (tstate) && NILP (self->error_symbol)) - sys_cond_wait (self->wait_condvar, &global_lock); + sys_cond_wait (self->wait_condvar, &global_lock, -1); self->wait_condvar = NULL; self->event_object = Qnil; --=-=-=--