[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Help-smalltalk] [PATCH] libgst: Fix SIGALRM race and cancel a timer
From: |
Paolo Bonzini |
Subject: |
Re: [Help-smalltalk] [PATCH] libgst: Fix SIGALRM race and cancel a timer before starting a new one |
Date: |
Fri, 26 Sep 2014 16:52:41 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 |
Il 26/09/2014 16:21, Holger Hans Peter Freyther ha scritto:
> From: Holger Hans Peter Freyther <address@hidden>
>
> The below Smalltalk code could trigger a race condition. We would program
> a timer and then program another one before the first has expired. If the
> original timer expires after we have set signal handler but before the
> syscall to set the new timer was called. We could receive SIGALARM and
> our signal handler would set SIG_DFL as the handler for SIGALARM. When
> the new expiration is set and expires the default handler will terminate
> the process.
>
> Setting the application to SIG_IGN the handled signal could lead to a
> deadlock as the real expiration is never signalled to the Smalltalk
> side.
>
> The best approach seems to cancel the timer. Before we have canceled
> the timer we might run through the signal handler and signal the
> original sempahore but that appears to be fine as we will not miss
> an event or revert the signal handler too early. It might be best to
> combine "TimeoutSem initialize" of Delay class>>#handleDelayRequstor
> with the cancelation of the timer.
Thanks, this is okay. It is racy anyway whether the
previously-registered timer would happen anyway; canceling the timer
solves the race in favor of the new timer.
Paolo
> I have only verified the working of the posix timer (timer_settime)
> and not the old interface.
>
> Eval [
> | sem a |
> sem := Semaphore new.
>
> a := [
> [
> (Delay forMilliseconds: 250) wait.
> sem signal.
> ] repeat.
> ] fork.
>
> b := [
> [
> (Delay forMilliseconds: 125) timedWaitOn: sem.
> ] repeat.
> ] fork.
>
> c := [
> [
> (Delay forMilliseconds: 125) timedWaitOn: sem.
> ] repeat.
> ] fork.
>
> stdin next.
> ]
> ---
> libgst/ChangeLog | 6 ++++++
> libgst/sysdep.h | 4 ++++
> libgst/sysdep/posix/events.c | 1 +
> libgst/sysdep/posix/timer.c | 19 +++++++++++++++++++
> 4 files changed, 30 insertions(+)
>
> diff --git a/libgst/ChangeLog b/libgst/ChangeLog
> index a390754..ce90b25 100644
> --- a/libgst/ChangeLog
> +++ b/libgst/ChangeLog
> @@ -1,3 +1,9 @@
> +2014-09-26 Holger Hans Peter Freyther <address@hidden>
> +
> + * sysdep.h: Declare _gst_sigalrm_cancel.
> + * sysdep/posix/events.c: Call _gst_sigalrm_cancel.
> + * sysdep/posix/timer.c: Implement _gst_sigalrm_cancel.
> +
> 2014-08-02 Holger Hans Peter Freyther <address@hidden>
>
> * prims.def: Introduce VMpr_Behavior_newInitialize and
> diff --git a/libgst/sysdep.h b/libgst/sysdep.h
> index d872c32..5648ef2 100644
> --- a/libgst/sysdep.h
> +++ b/libgst/sysdep.h
> @@ -108,6 +108,10 @@ extern void _gst_sigvtalrm_every (int deltaMilli,
> SigHandler func)
> ATTRIBUTE_HIDDEN;
>
> +/* Cancel an established SIGALRM */
> +extern void _gst_sigalrm_cancel (void)
> + ATTRIBUTE_HIDDEN;
> +
> /* Establish SIGALRM to be called when the nanosecond clock reaches NSTIME
> nanoseconds. */
> extern void _gst_sigalrm_at (int64_t nsTime)
> diff --git a/libgst/sysdep/posix/events.c b/libgst/sysdep/posix/events.c
> index 2525b37..39d6ea9 100644
> --- a/libgst/sysdep/posix/events.c
> +++ b/libgst/sysdep/posix/events.c
> @@ -200,6 +200,7 @@ void
> _gst_async_timed_wait (OOP semaphoreOOP,
> int64_t milliTime)
> {
> + _gst_sigalrm_cancel ();
> _gst_async_interrupt_wait (semaphoreOOP, SIGALRM);
> _gst_sigalrm_at (milliTime);
> }
> diff --git a/libgst/sysdep/posix/timer.c b/libgst/sysdep/posix/timer.c
> index 9c8fe28..c47ae91 100644
> --- a/libgst/sysdep/posix/timer.c
> +++ b/libgst/sysdep/posix/timer.c
> @@ -83,6 +83,25 @@ static mst_Boolean have_timer;
> #endif
>
> void
> +_gst_sigalrm_cancel ()
> +{
> +#ifdef HAVE_TIMER_CREATE
> + if (have_timer)
> + {
> + struct itimerspec value;
> + memset(&value, 0, sizeof(value));
> + timer_settime (timer, TIMER_ABSTIME, &value, NULL);
> + }
> + else
> +#endif
> + {
> + struct itimerval value;
> + memset(&value, 0, sizeof(value));
> + setitimer (ITIMER_REAL, &value, (struct itimerval *) 0);
> + }
> +}
> +
> +void
> _gst_sigalrm_at (int64_t nsTime)
> {
> #ifdef HAVE_TIMER_CREATE
>
Paolo