From e0eacb846c4c87b70a77095c4b50fe4c06255fec Mon Sep 17 00:00:00 2001 From: inmarket Date: Sat, 19 Nov 2016 19:39:04 +1000 Subject: [PATCH] Fixes to GEVENT --- docs/releases.txt | 1 + src/gevent/gevent.c | 86 +++++++++++++++++++-------------------------- src/gevent/gevent.h | 7 ++-- 3 files changed, 42 insertions(+), 52 deletions(-) diff --git a/docs/releases.txt b/docs/releases.txt index 02d85879..ee6b5ada 100644 --- a/docs/releases.txt +++ b/docs/releases.txt @@ -23,6 +23,7 @@ FEATURE: Added high-level functions to modify image color palettes FIX: Improving gdispDrawThickLine() FEATURE: Added gdispAddFont() for adding a dynamic font to the permanent font list FEATURE: Added gmiscHittestPoly() for checking whether a point is inside of a polygon +FIX: Fixed strange multi-thread issues in GEVENT *** Release 2.6 *** diff --git a/src/gevent/gevent.c b/src/gevent/gevent.c index 86eb23f0..733856c8 100644 --- a/src/gevent/gevent.c +++ b/src/gevent/gevent.c @@ -16,10 +16,8 @@ #endif /* Flags in the listener structure */ -#define GLISTENER_EVENTBUSY 0x0001 // The event buffer is busy -#define GLISTENER_WAITING 0x0002 // The listener is waiting for a signal -#define GLISTENER_WITHSOURCE 0x0004 // The listener is being looked at by a source for a possible event -#define GLISTENER_PENDING 0x0008 // There is an event waiting ready to go without a current listener +#define GLISTENER_WITHLISTENER 0x0001 // The listener is current using the buffer +#define GLISTENER_WITHSOURCE 0x0002 // The source is currently using the buffer /* This mutex protects access to our tables */ static gfxMutex geventMutex; @@ -30,11 +28,10 @@ static GSourceListener Assignments[GEVENT_MAX_SOURCE_LISTENERS]; /* Send an exit event if possible. */ /* We already have the geventMutex */ static void doExitEvent(GListener *pl) { - // Don't do the exit if someone else currently has the event lock - if ((pl->flags & (GLISTENER_WAITING|GLISTENER_EVENTBUSY)) == GLISTENER_WAITING) { - pl->flags |= GLISTENER_EVENTBUSY; // Event buffer is in use + // Don't do the exit if someone else currently is using the buffer + if (!(pl->flags & GLISTENER_WITHLISTENER)) { pl->event.type = GEVENT_EXIT; // Set up the EXIT event - pl->flags &= ~GLISTENER_WAITING; // Wake up the listener (with data) + pl->flags = GLISTENER_WITHLISTENER; // Event buffer is now in use by the listener gfxSemSignal(&pl->waitqueue); } } @@ -119,38 +116,36 @@ void geventDetachSource(GListener *pl, GSourceHandle gsh) { } GEvent *geventEventWait(GListener *pl, delaytime_t timeout) { - gfxMutexEnter(&geventMutex); - // Don't allow waiting if we are on callbacks or if there is already a thread waiting - if (pl->callback || (pl->flags & GLISTENER_WAITING)) { - gfxMutexExit(&geventMutex); + /* NOTE: + We no longer try to protect against two threads trying to listen on the + one listener. This was never allowed, it makes little sense to try to do so, + and the testing caused strange multi-thread windows of opportunity. + + In practice it is probably safer than it used to be - the only potential + issue is that the buffer may be prematurely marked as not in use by the listener. + If the calling code can guarantee that the event buffer is free when either thread + calls the event wait - it is now safe for them to do so. + ie. it is the implicit geventEventComplete() that is the only thing that now raises + possible multi-thread issues. + */ + + // Don't allow waiting if we are on callbacks + if (pl->callback) return 0; - } + + // Event buffer is not in use by the listener - this is an implicit geventEventComplete() call + pl->flags &= ~GLISTENER_WITHLISTENER; - // Check to see if there is a pending event ready for us - if ((pl->flags & GLISTENER_PENDING)) { - pl->flags &= ~GLISTENER_PENDING; // We have now got this - pl->flags |= GLISTENER_EVENTBUSY; // Event buffer is definitely busy - gfxMutexExit(&geventMutex); - return &pl->event; - } + // Wait for an event + if (!gfxSemWait(&pl->waitqueue, timeout)) + return 0; // Timeout - // No - wait for one. - pl->flags &= ~GLISTENER_EVENTBUSY; // Event buffer is definitely not busy - pl->flags |= GLISTENER_WAITING; // We will now be waiting on the thread - gfxMutexExit(&geventMutex); - if (gfxSemWait(&pl->waitqueue, timeout)) - return &pl->event; - - // Timeout - clear the waiting flag. - // We know this is safe as any other thread will still think there is someone waiting. - gfxMutexEnter(&geventMutex); - pl->flags &= ~GLISTENER_WAITING; - gfxMutexExit(&geventMutex); - return 0; + return &pl->event; } void geventEventComplete(GListener *pl) { - pl->flags &= ~GLISTENER_EVENTBUSY; + // The listener is done with the buffer + pl->flags &= ~GLISTENER_WITHLISTENER; } void geventRegisterCallback(GListener *pl, GEventCallbackFn fn, void *param) { @@ -160,7 +155,7 @@ void geventRegisterCallback(GListener *pl, GEventCallbackFn fn, void *param) { pl->param = param; // Set the param pl->callback = fn; // Set the callback function if (fn) - pl->flags &= ~GLISTENER_EVENTBUSY; // The event buffer is immediately available + pl->flags &= ~GLISTENER_WITHLISTENER; // The event buffer is immediately available gfxMutexExit(&geventMutex); } } @@ -176,7 +171,7 @@ GSourceListener *geventGetSourceListener(GSourceHandle gsh, GSourceListener *las // Unlock the last listener event buffer if it wasn't used. if (lastlr && lastlr->pListener && (lastlr->pListener->flags & GLISTENER_WITHSOURCE)) - lastlr->pListener->flags &= ~(GLISTENER_WITHSOURCE|GLISTENER_EVENTBUSY); + lastlr->pListener->flags &= ~GLISTENER_WITHSOURCE; // Loop through the table looking for attachments to this source for(psl = lastlr ? (lastlr+1) : Assignments; psl < Assignments+GEVENT_MAX_SOURCE_LISTENERS; psl++) { @@ -191,14 +186,13 @@ GSourceListener *geventGetSourceListener(GSourceHandle gsh, GSourceListener *las GEvent *geventGetEventBuffer(GSourceListener *psl) { gfxMutexEnter(&geventMutex); - if ((psl->pListener->flags & GLISTENER_EVENTBUSY)) { - // Oops - event buffer is still in use + if ((psl->pListener->flags & (GLISTENER_WITHLISTENER|GLISTENER_WITHSOURCE))) { gfxMutexExit(&geventMutex); return 0; } - // Allocate the event buffer - psl->pListener->flags |= (GLISTENER_WITHSOURCE|GLISTENER_EVENTBUSY); + // Allocate the event buffer to the source + psl->pListener->flags |= GLISTENER_WITHSOURCE; gfxMutexExit(&geventMutex); return &psl->pListener->event; } @@ -209,7 +203,7 @@ void geventSendEvent(GSourceListener *psl) { // Mark it back as free and as sent. This is early to be marking as free but it protects // if the callback alters the listener in any way - psl->pListener->flags &= ~(GLISTENER_WITHSOURCE|GLISTENER_EVENTBUSY|GLISTENER_PENDING); + psl->pListener->flags = 0; gfxMutexExit(&geventMutex); // Do the callback @@ -217,14 +211,8 @@ void geventSendEvent(GSourceListener *psl) { } else { // Wake up the listener - psl->pListener->flags &= ~GLISTENER_WITHSOURCE; - if ((psl->pListener->flags & GLISTENER_WAITING)) { - psl->pListener->flags &= ~(GLISTENER_WAITING|GLISTENER_PENDING); - gfxSemSignal(&psl->pListener->waitqueue); - } else - psl->pListener->flags |= GLISTENER_PENDING; - - // The listener thread will free the event buffer when ready + psl->pListener->flags = GLISTENER_WITHLISTENER; + gfxSemSignal(&psl->pListener->waitqueue); gfxMutexExit(&geventMutex); } } diff --git a/src/gevent/gevent.h b/src/gevent/gevent.h index a2b962ec..3c5a0bd4 100644 --- a/src/gevent/gevent.h +++ b/src/gevent/gevent.h @@ -146,7 +146,6 @@ void geventDetachSource(GListener *pl, GSourceHandle gsh); * @brief Wait for an event on a listener from an assigned source. * @details The type of the event should be checked (pevent->type) and then pevent should * be typecast to the actual event type if it needs to be processed. - * timeout specifies the time to wait in system ticks. * TIME_INFINITE means no timeout - wait forever for an event. * TIME_IMMEDIATE means return immediately * @note The returned GEvent is released when this routine is called again @@ -154,9 +153,11 @@ void geventDetachSource(GListener *pl, GSourceHandle gsh); * allows the GEvent object to be reused earlier which can reduce missed events. The GEvent * object MUST NOT be used after this function is called (and is blocked waiting for the next * event) or after geventEventComplete() is called. + * The one listener object should not be waited on using more than one thread simultanously + * because of the implicit geventEventComplete() that occurs when this function is called. * * @param[in] pl The listener - * @param[in] timeout The timeout + * @param[in] timeout The timeout in milliseconds * * @return NULL on timeout */ @@ -216,7 +217,7 @@ GSourceListener *geventGetSourceListener(GSourceHandle gsh, GSourceListener *las * * @param[in] psl The source listener * - * @return NULL if the listener is not currently listening. + * @return NULL if the event buffer for this listener is currently in use. */ GEvent *geventGetEventBuffer(GSourceListener *psl);