Improved sensor thread-safety

This fixes an issue where the main thread would hang indefinitely when polling events if sensors were initialized and shutdown on another thread.

src/sensor/SDL_sensor.c

@@ -49,18 +49,79 @@ static SDL_SensorDriver *SDL_sensor_drivers[] = {
     &SDL_DUMMY_SensorDriver
 #endif
 };
-static SDL_Mutex *SDL_sensor_lock = NULL; /* This needs to support recursive locks */
+
+#ifndef SDL_THREAD_SAFETY_ANALYSIS
+static
+#endif
+SDL_Mutex *SDL_sensor_lock = NULL; /* This needs to support recursive locks */
+static SDL_AtomicInt SDL_sensor_lock_pending;
+static int SDL_sensors_locked;
+static SDL_bool SDL_sensors_initialized;
 static SDL_Sensor *SDL_sensors SDL_GUARDED_BY(SDL_sensor_lock) = NULL;
 static SDL_AtomicInt SDL_last_sensor_instance_id SDL_GUARDED_BY(SDL_sensor_lock);
+char SDL_sensor_magic;
+
+#define CHECK_SENSOR_MAGIC(sensor, retval)              \
+    if (!sensor || sensor->magic != &SDL_sensor_magic) { \
+        SDL_InvalidParamError("sensor");                \
+        SDL_UnlockSensors();                            \
+        return retval;                                  \
+    }
+
+SDL_bool SDL_SensorsInitialized(void)
+{
+    return SDL_sensors_initialized;
+}
 
-void SDL_LockSensors(void) SDL_ACQUIRE(SDL_sensor_lock)
+void SDL_LockSensors(void)
 {
+    (void)SDL_AtomicIncRef(&SDL_sensor_lock_pending);
     SDL_LockMutex(SDL_sensor_lock);
+    (void)SDL_AtomicDecRef(&SDL_sensor_lock_pending);
+
+    ++SDL_sensors_locked;
+}
+
+void SDL_UnlockSensors(void)
+{
+    SDL_bool last_unlock = SDL_FALSE;
+
+    --SDL_sensors_locked;
+
+    if (!SDL_sensors_initialized) {
+        /* NOTE: There's a small window here where another thread could lock the mutex after we've checked for pending locks */
+        if (!SDL_sensors_locked && SDL_AtomicGet(&SDL_sensor_lock_pending) == 0) {
+            last_unlock = SDL_TRUE;
+        }
+    }
+
+    /* The last unlock after sensors are uninitialized will cleanup the mutex,
+     * allowing applications to lock sensors while reinitializing the system.
+     */
+    if (last_unlock) {
+        SDL_Mutex *sensor_lock = SDL_sensor_lock;
+
+        SDL_LockMutex(sensor_lock);
+        {
+            SDL_UnlockMutex(SDL_sensor_lock);
+
+            SDL_sensor_lock = NULL;
+        }
+        SDL_UnlockMutex(sensor_lock);
+        SDL_DestroyMutex(sensor_lock);
+    } else {
+        SDL_UnlockMutex(SDL_sensor_lock);
+    }
+}
+
+SDL_bool SDL_SensorsLocked(void)
+{
+    return (SDL_sensors_locked > 0) ? SDL_TRUE : SDL_FALSE;
 }
 
-void SDL_UnlockSensors(void) SDL_RELEASE(SDL_sensor_lock)
+void SDL_AssertSensorsLocked(void)
 {
-    SDL_UnlockMutex(SDL_sensor_lock);
+    SDL_assert(SDL_SensorsLocked());
 }
 
 int SDL_InitSensors(void)
@@ -78,12 +139,23 @@ int SDL_InitSensors(void)
     }
 #endif /* !SDL_EVENTS_DISABLED */
 
+    SDL_LockSensors();
+
+    SDL_sensors_initialized = SDL_TRUE;
+
     status = -1;
     for (i = 0; i < SDL_arraysize(SDL_sensor_drivers); ++i) {
         if (SDL_sensor_drivers[i]->Init() >= 0) {
             status = 0;
         }
     }
+
+    SDL_UnlockSensors();
+
+    if (status < 0) {
+        SDL_QuitSensors();
+    }
+
     return status;
 }
 
@@ -320,33 +392,22 @@ SDL_Sensor *SDL_GetSensorFromInstanceID(SDL_SensorID instance_id)
     return sensor;
 }
 
-/*
- * Checks to make sure the sensor is valid.
- */
-static int SDL_IsSensorValid(SDL_Sensor *sensor)
-{
-    int valid;
-
-    if (sensor == NULL) {
-        SDL_SetError("Sensor hasn't been opened yet");
-        valid = 0;
-    } else {
-        valid = 1;
-    }
-
-    return valid;
-}
-
 /*
  * Get the friendly name of this sensor
  */
 const char *SDL_GetSensorName(SDL_Sensor *sensor)
 {
-    if (!SDL_IsSensorValid(sensor)) {
-        return NULL;
+    const char *retval;
+
+    SDL_LockSensors();
+    {
+        CHECK_SENSOR_MAGIC(sensor, NULL);
+
+        retval = sensor->name;
     }
+    SDL_UnlockSensors();
 
-    return sensor->name;
+    return retval;
 }
 
 /*
@@ -354,11 +415,17 @@ const char *SDL_GetSensorName(SDL_Sensor *sensor)
  */
 SDL_SensorType SDL_GetSensorType(SDL_Sensor *sensor)
 {
-    if (!SDL_IsSensorValid(sensor)) {
-        return SDL_SENSOR_INVALID;
+    SDL_SensorType retval;
+
+    SDL_LockSensors();
+    {
+        CHECK_SENSOR_MAGIC(sensor, SDL_SENSOR_INVALID);
+
+        retval = sensor->type;
     }
+    SDL_UnlockSensors();
 
-    return sensor->type;
+    return retval;
 }
 
 /*
@@ -366,11 +433,17 @@ SDL_SensorType SDL_GetSensorType(SDL_Sensor *sensor)
  */
 int SDL_GetSensorNonPortableType(SDL_Sensor *sensor)
 {
-    if (!SDL_IsSensorValid(sensor)) {
-        return -1;
+    int retval;
+
+    SDL_LockSensors();
+    {
+        CHECK_SENSOR_MAGIC(sensor, -1);
+
+        retval = sensor->non_portable_type;
     }
+    SDL_UnlockSensors();
 
-    return sensor->non_portable_type;
+    return retval;
 }
 
 /*
@@ -378,11 +451,17 @@ int SDL_GetSensorNonPortableType(SDL_Sensor *sensor)
  */
 SDL_SensorID SDL_GetSensorInstanceID(SDL_Sensor *sensor)
 {
-    if (!SDL_IsSensorValid(sensor)) {
-        return 0;
+    SDL_SensorID retval;
+
+    SDL_LockSensors();
+    {
+        CHECK_SENSOR_MAGIC(sensor, 0);
+
+        retval = sensor->instance_id;
     }
+    SDL_UnlockSensors();
 
-    return sensor->instance_id;
+    return retval;
 }
 
 /*
@@ -390,12 +469,15 @@ SDL_SensorID SDL_GetSensorInstanceID(SDL_Sensor *sensor)
  */
 int SDL_GetSensorData(SDL_Sensor *sensor, float *data, int num_values)
 {
-    if (!SDL_IsSensorValid(sensor)) {
-        return -1;
+    SDL_LockSensors();
+    {
+        CHECK_SENSOR_MAGIC(sensor, -1);
+
+        num_values = SDL_min(num_values, SDL_arraysize(sensor->data));
+        SDL_memcpy(data, sensor->data, num_values * sizeof(*data));
     }
+    SDL_UnlockSensors();
 
-    num_values = SDL_min(num_values, SDL_arraysize(sensor->data));
-    SDL_memcpy(data, sensor->data, num_values * sizeof(*data));
     return 0;
 }
 
@@ -407,42 +489,39 @@ void SDL_CloseSensor(SDL_Sensor *sensor)
     SDL_Sensor *sensorlist;
     SDL_Sensor *sensorlistprev;
 
-    if (!SDL_IsSensorValid(sensor)) {
-        return;
-    }
-
     SDL_LockSensors();
+    {
+        CHECK_SENSOR_MAGIC(sensor,);
 
-    /* First decrement ref count */
-    if (--sensor->ref_count > 0) {
-        SDL_UnlockSensors();
-        return;
-    }
-
-    sensor->driver->Close(sensor);
-    sensor->hwdata = NULL;
+        /* First decrement ref count */
+        if (--sensor->ref_count > 0) {
+            SDL_UnlockSensors();
+            return;
+        }
 
-    sensorlist = SDL_sensors;
-    sensorlistprev = NULL;
-    while (sensorlist) {
-        if (sensor == sensorlist) {
-            if (sensorlistprev) {
-                /* unlink this entry */
-                sensorlistprev->next = sensorlist->next;
-            } else {
-                SDL_sensors = sensor->next;
+        sensor->driver->Close(sensor);
+        sensor->hwdata = NULL;
+
+        sensorlist = SDL_sensors;
+        sensorlistprev = NULL;
+        while (sensorlist) {
+            if (sensor == sensorlist) {
+                if (sensorlistprev) {
+                    /* unlink this entry */
+                    sensorlistprev->next = sensorlist->next;
+                } else {
+                    SDL_sensors = sensor->next;
+                }
+                break;
             }
-            break;
+            sensorlistprev = sensorlist;
+            sensorlist = sensorlist->next;
         }
-        sensorlistprev = sensorlist;
-        sensorlist = sensorlist->next;
-    }
-
-    SDL_free(sensor->name);
-
-    /* Free the data associated with this sensor */
-    SDL_free(sensor);
 
+        /* Free the data associated with this sensor */
+        SDL_free(sensor->name);
+        SDL_free(sensor);
+    }
     SDL_UnlockSensors();
 }
 
@@ -463,16 +542,13 @@ void SDL_QuitSensors(void)
         SDL_sensor_drivers[i]->Quit();
     }
 
-    SDL_UnlockSensors();
-
 #ifndef SDL_EVENTS_DISABLED
     SDL_QuitSubSystem(SDL_INIT_EVENTS);
 #endif
 
-    if (SDL_sensor_lock) {
-        SDL_DestroyMutex(SDL_sensor_lock);
-        SDL_sensor_lock = NULL;
-    }
+    SDL_sensors_initialized = SDL_FALSE;
+
+    SDL_UnlockSensors();
 }
 
 /* These are global for SDL_syssensor.c and SDL_events.c */
@@ -481,6 +557,8 @@ int SDL_SendSensorUpdate(Uint64 timestamp, SDL_Sensor *sensor, Uint64 sensor_tim
 {
     int posted;
 
+    SDL_AssertSensorsLocked();
+
     /* Allow duplicate events, for things like steps and heartbeats */
 
     /* Update internal sensor state */
@@ -510,7 +588,13 @@ int SDL_SendSensorUpdate(Uint64 timestamp, SDL_Sensor *sensor, Uint64 sensor_tim
 
 void SDL_UpdateSensor(SDL_Sensor *sensor)
 {
-    sensor->driver->Update(sensor);
+    SDL_LockSensors();
+    {
+        CHECK_SENSOR_MAGIC(sensor,);
+
+        sensor->driver->Update(sensor);
+    }
+    SDL_UnlockSensors();
 }
 
 void SDL_UpdateSensors(void)

src/sensor/SDL_sensor_c.h

@@ -23,6 +23,10 @@
 #ifndef SDL_sensor_c_h_
 #define SDL_sensor_c_h_
 
+#ifdef SDL_THREAD_SAFETY_ANALYSIS
+extern SDL_Mutex *SDL_sensor_lock;
+#endif
+
 struct SDL_SensorDriver;
 
 /* Useful functions and variables from SDL_sensor.c */
@@ -34,8 +38,17 @@ extern SDL_SensorID SDL_GetNextSensorInstanceID(void);
 extern int SDL_InitSensors(void);
 extern void SDL_QuitSensors(void);
 
-extern void SDL_LockSensors(void);
-extern void SDL_UnlockSensors(void);
+/* Return whether the sensor system is currently initialized */
+extern SDL_bool SDL_SensorsInitialized(void);
+
+/* Return whether the sensors are currently locked */
+extern SDL_bool SDL_SensorsLocked(void);
+
+/* Make sure we currently have the sensors locked */
+extern void SDL_AssertSensorsLocked(void) SDL_ASSERT_CAPABILITY(SDL_sensor_lock);
+
+extern void SDL_LockSensors(void) SDL_ACQUIRE(SDL_sensor_lock);
+extern void SDL_UnlockSensors(void) SDL_RELEASE(SDL_sensor_lock);
 
 /* Function to return whether there are any sensors opened by the application */
 extern SDL_bool SDL_SensorsOpened(void);

src/sensor/SDL_syssensor.h

@@ -27,25 +27,31 @@
 
 #include "SDL_sensor_c.h"
 
+#define _guarded SDL_GUARDED_BY(SDL_sensor_lock)
+
 /* The SDL sensor structure */
 struct SDL_Sensor
 {
-    SDL_SensorID instance_id;   /* Device instance, monotonically increasing from 0 */
-    char *name;                 /* Sensor name - system dependent */
-    SDL_SensorType type;        /* Type of the sensor */
-    int non_portable_type;      /* Platform dependent type of the sensor */
+    const void *magic _guarded;
+
+    SDL_SensorID instance_id _guarded;   /* Device instance, monotonically increasing from 0 */
+    char *name _guarded;                 /* Sensor name - system dependent */
+    SDL_SensorType type _guarded;        /* Type of the sensor */
+    int non_portable_type _guarded;      /* Platform dependent type of the sensor */
 
-    float data[16];             /* The current state of the sensor */
+    float data[16] _guarded;             /* The current state of the sensor */
 
-    struct SDL_SensorDriver *driver;
+    struct SDL_SensorDriver *driver _guarded;
 
-    struct sensor_hwdata *hwdata; /* Driver dependent information */
+    struct sensor_hwdata *hwdata _guarded; /* Driver dependent information */
 
-    int ref_count; /* Reference count for multiple opens */
+    int ref_count _guarded; /* Reference count for multiple opens */
 
-    struct SDL_Sensor *next; /* pointer to next sensor we have allocated */
+    struct SDL_Sensor *next _guarded; /* pointer to next sensor we have allocated */
 };
 
+#undef _guarded
+
 typedef struct SDL_SensorDriver
 {
     /* Function to scan the system for sensors.

src/sensor/android/SDL_androidsensor.c

@@ -52,7 +52,6 @@ typedef struct
 static ASensorManager *SDL_sensor_manager;
 static ALooper *SDL_sensor_looper;
 static SDL_AndroidSensorThreadContext SDL_sensor_thread_context;
-static SDL_Mutex *SDL_sensors_lock;
 static SDL_AndroidSensor *SDL_sensors SDL_GUARDED_BY(SDL_sensors_lock);
 static int SDL_sensors_count;
 
@@ -72,7 +71,7 @@ static int SDLCALL SDL_ANDROID_SensorThread(void *data)
         Uint64 timestamp = SDL_GetTicksNS();
 
         if (ALooper_pollAll(-1, NULL, &events, (void **)&source) == LOOPER_ID_USER) {
-            SDL_LockMutex(SDL_sensors_lock);
+            SDL_LockSensors();
             for (i = 0; i < SDL_sensors_count; ++i) {
                 if (!SDL_sensors[i].event_queue) {
                     continue;
@@ -83,7 +82,7 @@ static int SDLCALL SDL_ANDROID_SensorThread(void *data)
                     SDL_SendSensorUpdate(timestamp, SDL_sensors[i].sensor, timestamp, event.data, SDL_arraysize(event.data));
                 }
             }
-            SDL_UnlockMutex(SDL_sensors_lock);
+            SDL_UnlockSensors();
         }
     }
 
@@ -138,11 +137,6 @@ static int SDL_ANDROID_SensorInit(void)
     int i, sensors_count;
     ASensorList sensors;
 
-    SDL_sensors_lock = SDL_CreateMutex();
-    if (!SDL_sensors_lock) {
-        return SDL_SetError("Couldn't create sensor lock");
-    }
-
     SDL_sensor_manager = ASensorManager_getInstance();
     if (SDL_sensor_manager == NULL) {
         return SDL_SetError("Couldn't create sensor manager");
@@ -209,19 +203,19 @@ static int SDL_ANDROID_SensorOpen(SDL_Sensor *sensor, int device_index)
 {
     int delay_us, min_delay_us;
 
-    SDL_LockMutex(SDL_sensors_lock);
+    SDL_LockSensors();
     {
         SDL_sensors[device_index].sensor = sensor;
         SDL_sensors[device_index].event_queue = ASensorManager_createEventQueue(SDL_sensor_manager, SDL_sensor_looper, LOOPER_ID_USER, NULL, NULL);
         if (!SDL_sensors[device_index].event_queue) {
-            SDL_UnlockMutex(SDL_sensors_lock);
+            SDL_UnlockSensors();
             return SDL_SetError("Couldn't create sensor event queue");
         }
 
         if (ASensorEventQueue_enableSensor(SDL_sensors[device_index].event_queue, SDL_sensors[device_index].asensor) < 0) {
             ASensorManager_destroyEventQueue(SDL_sensor_manager, SDL_sensors[device_index].event_queue);
             SDL_sensors[device_index].event_queue = NULL;
-            SDL_UnlockMutex(SDL_sensors_lock);
+            SDL_UnlockSensors();
             return SDL_SetError("Couldn't enable sensor");
         }
 
@@ -234,7 +228,7 @@ static int SDL_ANDROID_SensorOpen(SDL_Sensor *sensor, int device_index)
         }
         ASensorEventQueue_setEventRate(SDL_sensors[device_index].event_queue, SDL_sensors[device_index].asensor, delay_us);
     }
-    SDL_UnlockMutex(SDL_sensors_lock);
+    SDL_UnlockSensors();
 
     return 0;
 }
@@ -249,14 +243,14 @@ static void SDL_ANDROID_SensorClose(SDL_Sensor *sensor)
 
     for (i = 0; i < SDL_sensors_count; ++i) {
         if (SDL_sensors[i].sensor == sensor) {
-            SDL_LockMutex(SDL_sensors_lock);
+            SDL_LockSensors();
             {
                 ASensorEventQueue_disableSensor(SDL_sensors[i].event_queue, SDL_sensors[i].asensor);
                 ASensorManager_destroyEventQueue(SDL_sensor_manager, SDL_sensors[i].event_queue);
                 SDL_sensors[i].event_queue = NULL;
                 SDL_sensors[i].sensor = NULL;
             }
-            SDL_UnlockMutex(SDL_sensors_lock);
+            SDL_UnlockSensors();
             break;
         }
     }
@@ -271,11 +265,6 @@ static void SDL_ANDROID_SensorQuit(void)
         SDL_sensors = NULL;
         SDL_sensors_count = 0;
     }
-
-    if (SDL_sensors_lock) {
-        SDL_DestroyMutex(SDL_sensors_lock);
-        SDL_sensors_lock = NULL;
-    }
 }
 
 SDL_SensorDriver SDL_ANDROID_SensorDriver = {