From ad53d45452569292a42e5c120a2da8f32d7fdb7c Mon Sep 17 00:00:00 2001 From: culler Date: Sat, 6 Jun 2020 21:23:50 +0000 Subject: Address macOS hangs in Tcl_WaitForEvent. --- macosx/tclMacOSXNotify.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/macosx/tclMacOSXNotify.c b/macosx/tclMacOSXNotify.c index 9b7bd1a..21d95e5 100644 --- a/macosx/tclMacOSXNotify.c +++ b/macosx/tclMacOSXNotify.c @@ -1243,6 +1243,13 @@ Tcl_WaitForEvent( */ polling = 1; + + /* + * Set a small positive waitTime so that when the runloop is started + * it will process all of its sources. + */ + + waitTime = 0.005; } } @@ -1264,11 +1271,16 @@ Tcl_WaitForEvent( runLoopRunning = tsdPtr->runLoopRunning; tsdPtr->runLoopRunning = 1; - runLoopStatus = CFRunLoopRunInMode(tsdPtr->runLoopServicingEvents || - runLoopRunning ? tclEventsOnlyRunLoopMode : kCFRunLoopDefaultMode, - waitTime, TRUE); + runLoopStatus = CFRunLoopRunInMode( + tsdPtr->runLoopServicingEvents || runLoopRunning ? + tclEventsOnlyRunLoopMode : kCFRunLoopDefaultMode, + waitTime, TRUE); tsdPtr->runLoopRunning = runLoopRunning; + if (polling) { + return tsdPtr->runLoopSourcePerformed ? 0 : 1; + } + LOCK_NOTIFIER_TSD; tsdPtr->polling = 0; UNLOCK_NOTIFIER_TSD; @@ -1384,7 +1396,6 @@ UpdateWaitingListAndServiceEvents( void *info) { ThreadSpecificData *tsdPtr = info; - if (tsdPtr->sleeping) { return; } @@ -1407,16 +1418,6 @@ UpdateWaitingListAndServiceEvents( } tsdPtr->runLoopNestingLevel--; break; - case kCFRunLoopBeforeWaiting: - if (tsdPtr->runLoopTimer && !tsdPtr->runLoopServicingEvents && - (tsdPtr->runLoopNestingLevel > 1 - || !tsdPtr->runLoopRunning)) { - tsdPtr->runLoopServicingEvents = 1; - /* This call seems to simply force event processing through and prevents hangups that have long been observed with Tk-Cocoa. */ - Tcl_ServiceAll(); - tsdPtr->runLoopServicingEvents = 0; - } - break; default: break; } -- cgit v0.12 From b345d9a593684ec1fd404884947fa1b5298d1af9 Mon Sep 17 00:00:00 2001 From: culler Date: Sun, 7 Jun 2020 22:15:28 +0000 Subject: Code simplification and cleanup --- macosx/tclMacOSXNotify.c | 41 ++++++++++++++++------------------------- 1 file changed, 16 insertions(+), 25 deletions(-) diff --git a/macosx/tclMacOSXNotify.c b/macosx/tclMacOSXNotify.c index 21d95e5..6f27e64 100644 --- a/macosx/tclMacOSXNotify.c +++ b/macosx/tclMacOSXNotify.c @@ -494,7 +494,7 @@ Tcl_InitNotifier(void) bzero(&runLoopObserverContext, sizeof(CFRunLoopObserverContext)); runLoopObserverContext.info = tsdPtr; runLoopObserver = CFRunLoopObserverCreate(NULL, - kCFRunLoopEntry|kCFRunLoopExit|kCFRunLoopBeforeWaiting, TRUE, + kCFRunLoopEntry|kCFRunLoopExit, TRUE, LONG_MIN, UpdateWaitingListAndServiceEvents, &runLoopObserverContext); if (!runLoopObserver) { @@ -512,7 +512,7 @@ Tcl_InitNotifier(void) */ runLoopObserverTcl = CFRunLoopObserverCreate(NULL, - kCFRunLoopEntry|kCFRunLoopExit|kCFRunLoopBeforeWaiting, TRUE, + kCFRunLoopEntry|kCFRunLoopExit, TRUE, LONG_MIN, UpdateWaitingListAndServiceEvents, &runLoopObserverContext); if (!runLoopObserverTcl) { @@ -1222,6 +1222,10 @@ Tcl_WaitForEvent( Tcl_Panic("Tcl_WaitForEvent: Notifier not initialized"); } + /* + * A NULL timePtr means wait forever. + */ + if (timePtr) { Tcl_Time vTime = *timePtr; @@ -1235,26 +1239,18 @@ Tcl_WaitForEvent( tclScaleTimeProcPtr(&vTime, tclTimeClientData); waitTime = vTime.sec + 1.0e-6 * vTime.usec; } else { - /* - * Polling: pretend to wait for files and tell the notifier thread - * what we are doing. The notifier thread makes sure it goes - * through select with its select mask in the same state as ours - * currently is. We block until that happens. - */ - - polling = 1; /* - * Set a small positive waitTime so that when the runloop is started - * it will process all of its sources. + * The max block time was set to 0. */ - waitTime = 0.005; + polling = 1; + waitTime = 0; } } StartNotifierThread(); - + LOCK_NOTIFIER_TSD; tsdPtr->polling = polling; UNLOCK_NOTIFIER_TSD; @@ -1262,25 +1258,20 @@ Tcl_WaitForEvent( /* * If the Tcl runloop is already running (e.g. if Tcl_WaitForEvent was - * called recursively) or is servicing events via the runloop observer, - * re-run it in a custom runloop mode containing only the source for the - * notifier thread, otherwise wakeups from other sources added to the - * common runloop modes might get lost or 3rd party event handlers might - * get called when they do not expect to be. + * called recursively) start a new runloop in a custom runloop mode + * containing only the source for the notifier thread. Otherwise wakeups + * from other sources added to the common runloop mode might get lost or + * 3rd party event handlers might get called when they do not expect to + * be. */ runLoopRunning = tsdPtr->runLoopRunning; tsdPtr->runLoopRunning = 1; runLoopStatus = CFRunLoopRunInMode( - tsdPtr->runLoopServicingEvents || runLoopRunning ? - tclEventsOnlyRunLoopMode : kCFRunLoopDefaultMode, + runLoopRunning ? tclEventsOnlyRunLoopMode : kCFRunLoopDefaultMode, waitTime, TRUE); tsdPtr->runLoopRunning = runLoopRunning; - if (polling) { - return tsdPtr->runLoopSourcePerformed ? 0 : 1; - } - LOCK_NOTIFIER_TSD; tsdPtr->polling = 0; UNLOCK_NOTIFIER_TSD; -- cgit v0.12 From 476d33d6b64feb9beb632647c35619343d3fe732 Mon Sep 17 00:00:00 2001 From: culler Date: Thu, 18 Jun 2020 20:02:01 +0000 Subject: Sometimes the waitTime needs to be positive to avoid missing channel io events. --- macosx/tclMacOSXNotify.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/macosx/tclMacOSXNotify.c b/macosx/tclMacOSXNotify.c index 6f27e64..4ce6786 100644 --- a/macosx/tclMacOSXNotify.c +++ b/macosx/tclMacOSXNotify.c @@ -255,8 +255,6 @@ typedef struct ThreadSpecificData { int runLoopRunning; /* True if this thread's Tcl runLoop is * running. */ int runLoopNestingLevel; /* Level of nested runLoop invocations. */ - int runLoopServicingEvents; /* True if this thread's runLoop is servicing - * Tcl events. */ /* Must hold the notifierLock before accessing the following fields: */ /* Start notifierLock section */ @@ -1225,7 +1223,7 @@ Tcl_WaitForEvent( /* * A NULL timePtr means wait forever. */ - + if (timePtr) { Tcl_Time vTime = *timePtr; @@ -1242,15 +1240,25 @@ Tcl_WaitForEvent( /* * The max block time was set to 0. + * + * If we set the waitTime to 0, then the call to CFRunLoopInMode + * may return without processing all of its sources. The Apple + * documentation says that if the waitTime is 0 "only one pass is + * made through the run loop before returning; if multiple sources + * or timers are ready to fire immediately, only one (possibly two + * if one is a version 0 source) will be fired, regardless of the + * value of returnAfterSourceHandled." This can cause some chanio + * tests to fail. So we use a small positive waitTime unless there + * is another RunLoop running. */ polling = 1; - waitTime = 0; + waitTime = tsdPtr->runLoopRunning ? 0 : 0.0001; } } StartNotifierThread(); - + LOCK_NOTIFIER_TSD; tsdPtr->polling = polling; UNLOCK_NOTIFIER_TSD; -- cgit v0.12 From a0457efe80223c1380e9e0fc6a50c755fc7bbeda Mon Sep 17 00:00:00 2001 From: culler Date: Mon, 22 Jun 2020 16:12:00 +0000 Subject: Use the os_unfair_lock in place of OSSpinLock when the minimum build target is newer than OSX 10.12 --- macosx/tclMacOSXNotify.c | 70 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 10 deletions(-) diff --git a/macosx/tclMacOSXNotify.c b/macosx/tclMacOSXNotify.c index 4ce6786..1f9d0ea 100644 --- a/macosx/tclMacOSXNotify.c +++ b/macosx/tclMacOSXNotify.c @@ -14,6 +14,18 @@ */ #include "tclInt.h" + +/* + * In macOS 10.12 the os_unfair_lock was introduced as a replacement for the + * OSSpinLock, and the OSSpinLock was deprecated. + */ + +#if MAC_OS_X_VERSION_MIN_REQUIRED >= 101200 +#define USE_OS_UNFAIR_LOCK +#include +#undef TCL_MAC_DEBUG_NOTIFIER +#endif + #ifdef HAVE_COREFOUNDATION /* Traditional unix select-based notifier is * in tclUnixNotfy.c */ #include @@ -21,6 +33,8 @@ /* #define TCL_MAC_DEBUG_NOTIFIER 1 */ +#if !defined(USE_OS_UNFAIR_LOCK) + /* * We use the Darwin-native spinlock API rather than pthread mutexes for * notifier locking: this radically simplifies the implementation and lowers @@ -102,26 +116,45 @@ extern int _spin_lock_try(OSSpinLock *lock); #define SPINLOCK_INIT 0 #endif /* HAVE_LIBKERN_OSATOMIC_H && HAVE_OSSPINLOCKLOCK */ +#endif /* not using os_unfair_lock */ /* - * These spinlocks lock access to the global notifier state. + * These locks control access to the global notifier state. */ +#if defined(USE_OS_UNFAIR_LOCK) +static os_unfair_lock notifierInitLock = OS_UNFAIR_LOCK_INIT; +static os_unfair_lock notifierLock = OS_UNFAIR_LOCK_INIT; +#else static OSSpinLock notifierInitLock = SPINLOCK_INIT; static OSSpinLock notifierLock = SPINLOCK_INIT; +#endif /* - * Macros abstracting notifier locking/unlocking + * Macros that abstract notifier locking/unlocking */ +#if defined(USE_OS_UNFAIR_LOCK) +#define LOCK_NOTIFIER_INIT os_unfair_lock_lock(¬ifierInitLock) +#define UNLOCK_NOTIFIER_INIT os_unfair_lock_unlock(¬ifierInitLock) +#define LOCK_NOTIFIER os_unfair_lock_lock(¬ifierLock) +#define UNLOCK_NOTIFIER os_unfair_lock_unlock(¬ifierLock) +#define LOCK_NOTIFIER_TSD os_unfair_lock_lock(&tsdPtr->tsdLock) +#define UNLOCK_NOTIFIER_TSD os_unfair_lock_unlock(&tsdPtr->tsdLock) +#else #define LOCK_NOTIFIER_INIT SpinLockLock(¬ifierInitLock) #define UNLOCK_NOTIFIER_INIT SpinLockUnlock(¬ifierInitLock) #define LOCK_NOTIFIER SpinLockLock(¬ifierLock) #define UNLOCK_NOTIFIER SpinLockUnlock(¬ifierLock) #define LOCK_NOTIFIER_TSD SpinLockLock(&tsdPtr->tsdLock) #define UNLOCK_NOTIFIER_TSD SpinLockUnlock(&tsdPtr->tsdLock) +#endif -#ifdef TCL_MAC_DEBUG_NOTIFIER +/* + * The debug version of the Notifier only works if using OSSpinLock. + */ + +#if defined(TCL_MAC_DEBUG_NOTIFIER) && !defined(USE_OS_UNFAIR_LOCK) #define TclMacOSXNotifierDbgMsg(m, ...) \ do { \ fprintf(notifierLog?notifierLog:stderr, "tclMacOSXNotify.c:%d: " \ @@ -148,7 +181,7 @@ static OSSpinLock notifierLock = SPINLOCK_INIT; #undef LOCK_NOTIFIER #define LOCK_NOTIFIER SpinLockLockDbg(¬ifierLock) #undef LOCK_NOTIFIER_TSD -#define LOCK_NOTIFIER_TSD SpinLockLockDbg(&tsdPtr->tsdLock) +#define LOCK_NOTIFIER_TSD SpinLockLockDbg(tsdPtr->tsdLock) #include static FILE *notifierLog = NULL; #ifndef NOTIFIER_LOG @@ -267,9 +300,14 @@ typedef struct ThreadSpecificData { * from these pointers. */ /* End notifierLock section */ +#if defined(USE_OS_UNFAIR_LOCK) + os_unfair_lock tsdLock; +#else OSSpinLock tsdLock; /* Must hold this lock before acessing the * following fields from more than one * thread. */ +#endif + /* Start tsdLock section */ SelectMasks checkMasks; /* This structure is used to build up the * masks to be used in the next call to @@ -455,7 +493,6 @@ Tcl_InitNotifier(void) /* * Initialize support for weakly imported spinlock API. */ - if (pthread_once(&spinLockLockInitControl, SpinLockLockInit)) { Tcl_Panic("Tcl_InitNotifier: pthread_once failed"); } @@ -526,7 +563,11 @@ Tcl_InitNotifier(void) tsdPtr->runLoopObserverTcl = runLoopObserverTcl; tsdPtr->runLoopTimer = NULL; tsdPtr->waitTime = CF_TIMEINTERVAL_FOREVER; +#if defined(USE_OS_UNFAIR_LOCK) + tsdPtr->tsdLock = OS_UNFAIR_LOCK_INIT; +#else tsdPtr->tsdLock = SPINLOCK_INIT; +#endif } LOCK_NOTIFIER_INIT; @@ -584,7 +625,6 @@ Tcl_InitNotifier(void) ENABLE_ASL; notifierCount++; UNLOCK_NOTIFIER_INIT; - return tsdPtr; } @@ -1449,7 +1489,7 @@ OnOffWaitingList( { int changeWaitingList; -#ifdef TCL_MAC_DEBUG_NOTIFIER +#if defined(TCL_MAC_DEBUG_NOTIFIER) && !defined(USE_OS_UNFAIR_LOCK) if (SpinLockTry(¬ifierLock)) { Tcl_Panic("OnOffWaitingList: notifierLock unlocked"); } @@ -1980,9 +2020,19 @@ AtForkChild(void) { ThreadSpecificData *tsdPtr = TCL_TSD_INIT(&dataKey); - UNLOCK_NOTIFIER_TSD; - UNLOCK_NOTIFIER; - UNLOCK_NOTIFIER_INIT; + /* + * If a child process unlocks an os_unfair_lock that was created in its parent + * the child will exit with an illegal instruction error. So we reinitialize + * the lock in the child rather than attempt to unlock it. + */ + +#if defined(USE_OS_UNFAIR_LOCK) + tsdPtr->tsdLock = OS_UNFAIR_LOCK_INIT; +#else + UNLOCK_NOTIFIER_TSD; + UNLOCK_NOTIFIER; + UNLOCK_NOTIFIER_INIT; +#endif if (tsdPtr->runLoop) { tsdPtr->runLoop = NULL; if (!noCFafterFork) { -- cgit v0.12