Fixes to GEVENT

2 files changed, 41 insertions, 52 deletions

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;
-	}
-
-	// 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;
-	}
+		
+	// Event buffer is not in use by the listener - this is an implicit geventEventComplete() call
+	pl->flags &= ~GLISTENER_WITHLISTENER;
 
-	// 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;
+	// Wait for an event
+	if (!gfxSemWait(&pl->waitqueue, timeout))
+		return 0;				// Timeout
 
-	// 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);