Skip to content

Commit

Permalink
Removed external hashtable locking functions
Browse files Browse the repository at this point in the history 
Read-write locks are not recursive and can't be upgraded from one type to another, so it's not safe to lock the hash table and then call functions that operate on it. If you really want this functionality, we'd need to create unlocked versions of the hashtable functions and you would call those once you've taken a lock on the hashtable, and we'd have to assert that the operations you're doing are compatible with the type of lock you've taken.

All of that complicates working with hashtables, so if you need that type of access, you should probably just use external locking.
  • Loading branch information
@slouken
slouken committed Dec 12, 2024
1 parent 68f1f31 commit 835c8b0
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 56 deletions.
22 changes: 0 additions & 22 deletions src/SDL_hashtable.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -493,28 +493,6 @@ bool SDL_HashTableEmpty(SDL_HashTable *table)
return !(table && table->num_occupied_slots);
}

void SDL_LockHashTable(SDL_HashTable *table, bool for_writing)
{
if (!table) {
return;
}

if (for_writing) {
SDL_LockRWLockForWriting(table->lock);
} else {
SDL_LockRWLockForReading(table->lock);
}
}

void SDL_UnlockHashTable(SDL_HashTable *table)
{
if (!table) {
return;
}

SDL_UnlockRWLock(table->lock);
}

static void nuke_all(SDL_HashTable *table)
{
void *data = table->data;
Expand Down
7 changes: 2 additions & 5 deletions src/SDL_hashtable.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,12 @@ extern bool SDL_FindInHashTable(const SDL_HashTable *table, const void *key, con
// This function is thread-safe if the hashtable was created with threadsafe = true
extern bool SDL_HashTableEmpty(SDL_HashTable *table);

extern void SDL_LockHashTable(SDL_HashTable *table, bool for_writing);
extern void SDL_UnlockHashTable(SDL_HashTable *table);

// iterate all values for a specific key. This only makes sense if the hash is stackable. If not-stackable, just use SDL_FindInHashTable().
// This function is not thread-safe, you should use SDL_LockHashTable() if necessary
// This function is not thread-safe, you should use external locking if you use this function
extern bool SDL_IterateHashTableKey(const SDL_HashTable *table, const void *key, const void **_value, void **iter);

// iterate all key/value pairs in a hash (stackable hashes can have duplicate keys with multiple values).
// This function is not thread-safe, you should use SDL_LockHashTable() if necessary
// This function is not thread-safe, you should use external locking if you use this function
extern bool SDL_IterateHashTable(const SDL_HashTable *table, const void **_key, const void **_value, void **iter);

extern Uint32 SDL_HashPointer(const void *key, void *unused);
Expand Down
56 changes: 37 additions & 19 deletions src/camera/SDL_camera.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,9 +299,11 @@ void UnrefPhysicalCamera(SDL_Camera *device)
{
if (SDL_AtomicDecRef(&device->refcount)) {
// take it out of the device list.
SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
if (SDL_RemoveFromHashTable(camera_driver.device_hash, (const void *) (uintptr_t) device->instance_id)) {
SDL_AddAtomicInt(&camera_driver.device_count, -1);
}
SDL_UnlockRWLock(camera_driver.device_hash_lock);
DestroyPhysicalCamera(device); // ...and nuke it.
}
}
Expand All @@ -327,7 +329,9 @@ static SDL_Camera *ObtainPhysicalCamera(SDL_CameraID devid) // !!! FIXME: SDL_A
}

SDL_Camera *device = NULL;
SDL_LockRWLockForReading(camera_driver.device_hash_lock);
SDL_FindInHashTable(camera_driver.device_hash, (const void *) (uintptr_t) devid, (const void **) &device);
SDL_UnlockRWLock(camera_driver.device_hash_lock);
if (!device) {
SDL_SetError("Invalid camera device instance ID");
} else {
Expand Down Expand Up @@ -420,7 +424,9 @@ SDL_Camera *SDL_AddCamera(const char *name, SDL_CameraPosition position, int num
SDL_assert((specs != NULL) == (num_specs > 0));
SDL_assert(handle != NULL);

SDL_LockRWLockForReading(camera_driver.device_hash_lock);
const int shutting_down = SDL_GetAtomicInt(&camera_driver.shutting_down);
SDL_UnlockRWLock(camera_driver.device_hash_lock);
if (shutting_down) {
return NULL; // we're shutting down, don't add any devices that are hotplugged at the last possible moment.
}
Expand Down Expand Up @@ -490,6 +496,7 @@ SDL_Camera *SDL_AddCamera(const char *name, SDL_CameraPosition position, int num
SDL_SetAtomicInt(&device->zombie, 0);
RefPhysicalCamera(device);

SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
if (SDL_InsertIntoHashTable(camera_driver.device_hash, (const void *) (uintptr_t) device->instance_id, device)) {
SDL_AddAtomicInt(&camera_driver.device_count, 1);
} else {
Expand All @@ -507,14 +514,13 @@ SDL_Camera *SDL_AddCamera(const char *name, SDL_CameraPosition position, int num
p->type = SDL_EVENT_CAMERA_DEVICE_ADDED;
p->devid = device->instance_id;
p->next = NULL;
SDL_LockHashTable(camera_driver.device_hash, true);
SDL_assert(camera_driver.pending_events_tail != NULL);
SDL_assert(camera_driver.pending_events_tail->next == NULL);
camera_driver.pending_events_tail->next = p;
camera_driver.pending_events_tail = p;
SDL_UnlockHashTable(camera_driver.device_hash);
}
}
SDL_UnlockRWLock(camera_driver.device_hash_lock);

return device;
}
Expand Down Expand Up @@ -568,12 +574,12 @@ void SDL_CameraDisconnected(SDL_Camera *device)

if (first_disconnect) {
if (pending.next) { // NULL if event is disabled or disaster struck.
SDL_LockHashTable(camera_driver.device_hash, true);
SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
SDL_assert(camera_driver.pending_events_tail != NULL);
SDL_assert(camera_driver.pending_events_tail->next == NULL);
camera_driver.pending_events_tail->next = pending.next;
camera_driver.pending_events_tail = pending_tail;
SDL_UnlockHashTable(camera_driver.device_hash);
SDL_UnlockRWLock(camera_driver.device_hash_lock);
}
}
}
Expand Down Expand Up @@ -606,12 +612,12 @@ void SDL_CameraPermissionOutcome(SDL_Camera *device, bool approved)
ReleaseCamera(device);

if (pending.next) { // NULL if event is disabled or disaster struck.
SDL_LockHashTable(camera_driver.device_hash, true);
SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
SDL_assert(camera_driver.pending_events_tail != NULL);
SDL_assert(camera_driver.pending_events_tail->next == NULL);
camera_driver.pending_events_tail->next = pending.next;
camera_driver.pending_events_tail = pending_tail;
SDL_UnlockHashTable(camera_driver.device_hash);
SDL_UnlockRWLock(camera_driver.device_hash_lock);
}
}

Expand All @@ -627,15 +633,16 @@ SDL_Camera *SDL_FindPhysicalCameraByCallback(bool (*callback)(SDL_Camera *device
const void *value;
void *iter = NULL;

SDL_LockHashTable(camera_driver.device_hash, false);
SDL_LockRWLockForReading(camera_driver.device_hash_lock);
while (SDL_IterateHashTable(camera_driver.device_hash, &key, &value, &iter)) {
SDL_Camera *device = (SDL_Camera *) value;
if (callback(device, userdata)) { // found it?
SDL_UnlockHashTable(camera_driver.device_hash);
SDL_UnlockRWLock(camera_driver.device_hash_lock);
return device;
}
}
SDL_UnlockHashTable(camera_driver.device_hash);

SDL_UnlockRWLock(camera_driver.device_hash_lock);

SDL_SetError("Device not found");
return NULL;
Expand Down Expand Up @@ -709,7 +716,7 @@ SDL_CameraID *SDL_GetCameras(int *count)

SDL_CameraID *result = NULL;

SDL_LockHashTable(camera_driver.device_hash, false);
SDL_LockRWLockForReading(camera_driver.device_hash_lock);
int num_devices = SDL_GetAtomicInt(&camera_driver.device_count);
result = (SDL_CameraID *) SDL_malloc((num_devices + 1) * sizeof (SDL_CameraID));
if (!result) {
Expand All @@ -726,7 +733,7 @@ SDL_CameraID *SDL_GetCameras(int *count)
SDL_assert(devs_seen == num_devices);
result[devs_seen] = 0; // null-terminated.
}
SDL_UnlockHashTable(camera_driver.device_hash);
SDL_UnlockRWLock(camera_driver.device_hash_lock);

*count = num_devices;

Expand Down Expand Up @@ -1356,14 +1363,14 @@ void SDL_QuitCamera(void)
return;
}

SDL_HashTable *device_hash = camera_driver.device_hash;
SDL_LockHashTable(device_hash, true);
SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
SDL_SetAtomicInt(&camera_driver.shutting_down, 1);
SDL_HashTable *device_hash = camera_driver.device_hash;
camera_driver.device_hash = NULL;
SDL_PendingCameraEvent *pending_events = camera_driver.pending_events.next;
camera_driver.pending_events.next = NULL;
SDL_SetAtomicInt(&camera_driver.device_count, 0);
SDL_UnlockHashTable(device_hash);
SDL_UnlockRWLock(camera_driver.device_hash_lock);

SDL_PendingCameraEvent *pending_next = NULL;
for (SDL_PendingCameraEvent *i = pending_events; i; i = pending_next) {
Expand All @@ -1381,6 +1388,7 @@ void SDL_QuitCamera(void)
// Free the driver data
camera_driver.impl.Deinitialize();

SDL_DestroyRWLock(camera_driver.device_hash_lock);
SDL_DestroyHashTable(device_hash);

SDL_zero(camera_driver);
Expand Down Expand Up @@ -1409,8 +1417,14 @@ bool SDL_CameraInit(const char *driver_name)
SDL_QuitCamera(); // shutdown driver if already running.
}

SDL_HashTable *device_hash = SDL_CreateHashTable(NULL, 8, HashCameraID, MatchCameraID, NukeCameraHashItem, true, false);
SDL_RWLock *device_hash_lock = SDL_CreateRWLock(); // create this early, so if it fails we don't have to tear down the whole camera subsystem.
if (!device_hash_lock) {
return false;
}

SDL_HashTable *device_hash = SDL_CreateHashTable(NULL, 8, HashCameraID, MatchCameraID, NukeCameraHashItem, false, false);
if (!device_hash) {
SDL_DestroyRWLock(device_hash_lock);
return false;
}

Expand All @@ -1427,6 +1441,7 @@ bool SDL_CameraInit(const char *driver_name)
const char *driver_attempt = driver_name_copy;

if (!driver_name_copy) {
SDL_DestroyRWLock(device_hash_lock);
SDL_DestroyHashTable(device_hash);
return false;
}
Expand All @@ -1442,6 +1457,7 @@ bool SDL_CameraInit(const char *driver_name)
tried_to_init = true;
SDL_zero(camera_driver);
camera_driver.pending_events_tail = &camera_driver.pending_events;
camera_driver.device_hash_lock = device_hash_lock;
camera_driver.device_hash = device_hash;
if (bootstrap[i]->init(&camera_driver.impl)) {
camera_driver.name = bootstrap[i]->name;
Expand All @@ -1465,6 +1481,7 @@ bool SDL_CameraInit(const char *driver_name)
tried_to_init = true;
SDL_zero(camera_driver);
camera_driver.pending_events_tail = &camera_driver.pending_events;
camera_driver.device_hash_lock = device_hash_lock;
camera_driver.device_hash = device_hash;
if (bootstrap[i]->init(&camera_driver.impl)) {
camera_driver.name = bootstrap[i]->name;
Expand All @@ -1485,6 +1502,7 @@ bool SDL_CameraInit(const char *driver_name)
}

SDL_zero(camera_driver);
SDL_DestroyRWLock(device_hash_lock);
SDL_DestroyHashTable(device_hash);
return false; // No driver was available, so fail.
}
Expand All @@ -1501,20 +1519,20 @@ bool SDL_CameraInit(const char *driver_name)
// ("UpdateSubsystem" is the same naming that the other things that hook into PumpEvents use.)
void SDL_UpdateCamera(void)
{
SDL_LockHashTable(camera_driver.device_hash, false);
SDL_LockRWLockForReading(camera_driver.device_hash_lock);
SDL_PendingCameraEvent *pending_events = camera_driver.pending_events.next;
SDL_UnlockHashTable(camera_driver.device_hash);
SDL_UnlockRWLock(camera_driver.device_hash_lock);

if (!pending_events) {
return; // nothing to do, check next time.
}

// okay, let's take this whole list of events so we can dump the lock, and new ones can queue up for a later update.
SDL_LockHashTable(camera_driver.device_hash, true);
SDL_LockRWLockForWriting(camera_driver.device_hash_lock);
pending_events = camera_driver.pending_events.next; // in case this changed...
camera_driver.pending_events.next = NULL;
camera_driver.pending_events_tail = &camera_driver.pending_events;
SDL_UnlockHashTable(camera_driver.device_hash);
SDL_UnlockRWLock(camera_driver.device_hash_lock);

SDL_PendingCameraEvent *pending_next = NULL;
for (SDL_PendingCameraEvent *i = pending_events; i; i = pending_next) {
Expand Down
42 changes: 32 additions & 10 deletions src/stdlib/SDL_getenv.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ static char **environ;

struct SDL_Environment
{
SDL_Mutex *lock;
SDL_HashTable *strings;
};
static SDL_Environment *SDL_environment;
Expand Down Expand Up @@ -87,12 +88,15 @@ SDL_Environment *SDL_CreateEnvironment(bool populated)
return NULL;
}

env->strings = SDL_CreateHashTable(NULL, 16, SDL_HashString, SDL_KeyMatchString, SDL_NukeFreeKey, true, false);
env->strings = SDL_CreateHashTable(NULL, 16, SDL_HashString, SDL_KeyMatchString, SDL_NukeFreeKey, false, false);
if (!env->strings) {
SDL_free(env);
return NULL;
}

// Don't fail if we can't create a mutex (e.g. on a single-thread environment)
env->lock = SDL_CreateMutex();

if (populated) {
#ifdef SDL_PLATFORM_WINDOWS
LPWCH strings = GetEnvironmentStringsW();
Expand Down Expand Up @@ -153,10 +157,15 @@ const char *SDL_GetEnvironmentVariable(SDL_Environment *env, const char *name)
return NULL;
}

const char *value;
if (SDL_FindInHashTable(env->strings, name, (const void **)&value)) {
result = SDL_GetPersistentString(value);
SDL_LockMutex(env->lock);
{
const char *value;

if (SDL_FindInHashTable(env->strings, name, (const void **)&value)) {
result = SDL_GetPersistentString(value);
}
}
SDL_UnlockMutex(env->lock);

return result;
}
Expand All @@ -170,7 +179,7 @@ char **SDL_GetEnvironmentVariables(SDL_Environment *env)
return NULL;
}

SDL_LockHashTable(env->strings, false);
SDL_LockMutex(env->lock);
{
size_t count, length = 0;
void *iter;
Expand Down Expand Up @@ -207,7 +216,7 @@ char **SDL_GetEnvironmentVariables(SDL_Environment *env)
}
result[count] = NULL;
}
SDL_UnlockHashTable(env->strings);
SDL_UnlockMutex(env->lock);

return result;
}
Expand All @@ -224,7 +233,7 @@ bool SDL_SetEnvironmentVariable(SDL_Environment *env, const char *name, const ch
return SDL_InvalidParamError("value");
}

SDL_LockHashTable(env->strings, true);
SDL_LockMutex(env->lock);
{
const void *existing_value;
bool insert = true;
Expand All @@ -249,21 +258,33 @@ bool SDL_SetEnvironmentVariable(SDL_Environment *env, const char *name, const ch
}
}
}
SDL_UnlockHashTable(env->strings);
SDL_UnlockMutex(env->lock);

return result;
}

bool SDL_UnsetEnvironmentVariable(SDL_Environment *env, const char *name)
{
bool result = false;

if (!env) {
return SDL_InvalidParamError("env");
} else if (!name || *name == '\0' || SDL_strchr(name, '=') != NULL) {
return SDL_InvalidParamError("name");
}

SDL_RemoveFromHashTable(env->strings, name);
return true;
SDL_LockMutex(env->lock);
{
const void *value;
if (SDL_FindInHashTable(env->strings, name, &value)) {
result = SDL_RemoveFromHashTable(env->strings, name);
} else {
result = true;
}
}
SDL_UnlockMutex(env->lock);

return result;
}

void SDL_DestroyEnvironment(SDL_Environment *env)
Expand All @@ -272,6 +293,7 @@ void SDL_DestroyEnvironment(SDL_Environment *env)
return;
}

SDL_DestroyMutex(env->lock);
SDL_DestroyHashTable(env->strings);
SDL_free(env);
}
Expand Down

0 comments on commit 835c8b0

Please sign in to comment.