mike-jumper commented on code in PR #614: URL: https://github.com/apache/guacamole-server/pull/614#discussion_r2324360579
########## src/libguac/guacamole/thread-local.h: ########## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#ifndef GUAC_THREAD_LOCAL_H +#define GUAC_THREAD_LOCAL_H + +#include <stdint.h> + +/** + * Thread-local storage abstraction that provides pthread_key_t compatible + * interface but uses more efficient implementations where possible. + * + * This provides two different implementations: + * 1. Direct __thread storage for simple cases (like error handling) + * 2. Hash-table based storage for complex cases (like rwlock with process sharing) + */ + +/** + * Type representing a thread-local key, compatible with pthread_key_t usage patterns. + */ +typedef uintptr_t guac_thread_local_key_t; + +/** + * Destructor function type for thread-local storage cleanup. + */ +typedef void (*guac_thread_local_destructor_t)(void*); + +/** + * Once control structure for one-time initialization. + */ +typedef struct guac_thread_local_once_t { + volatile int done; + int mutex_init; + void* mutex_data; +} guac_thread_local_once_t; Review Comment: Please document the members of this structure. ########## src/libguac/error.c: ########## @@ -169,78 +169,21 @@ const char* guac_status_string(guac_status status) { #ifdef HAVE_LIBPTHREAD -/* PThread implementation of __guac_error */ +/* Thread-local implementation using __thread for optimal performance */ -static pthread_key_t __guac_error_key; -static pthread_once_t __guac_error_key_init = PTHREAD_ONCE_INIT; - -static pthread_key_t __guac_error_message_key; -static pthread_once_t __guac_error_message_key_init = PTHREAD_ONCE_INIT; - -static void __guac_mem_free_pointer(void* pointer) { - - /* Free memory allocated to status variable */ - guac_mem_free(pointer); - -} - -static void __guac_alloc_error_key() { - - /* Create key, destroy any allocated variable on thread exit */ - pthread_key_create(&__guac_error_key, __guac_mem_free_pointer); - -} - -static void __guac_alloc_error_message_key() { - - /* Create key, destroy any allocated variable on thread exit */ - pthread_key_create(&__guac_error_message_key, __guac_mem_free_pointer); - -} +/* + * Simple __thread implementation for error handling. + * Uses static allocation to avoid memory management complexity. + */ +static __thread guac_status __guac_error_storage; +static __thread const char* __guac_error_message_storage = NULL; Review Comment: Since `__thread` is a non-standard C compiler extension, I think we should test for whether the compiler supports this and fall back to standard mechanisms. ########## src/libguac/thread-local.c: ########## @@ -0,0 +1,203 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "config.h" +#include "guacamole/thread-local.h" + +#include <stdlib.h> +#include <pthread.h> +#include <errno.h> +#include <sched.h> + +/** + * Maximum number of thread-local keys supported. + */ +#define MAX_THREAD_KEYS 1024 + +/** + * Structure to hold destructor information for each key. + */ +typedef struct guac_key_entry { + guac_thread_local_destructor_t destructor; + int in_use; +} guac_key_entry_t; + +/** + * Thread-local storage entry. + */ +typedef struct guac_thread_storage { + void* values[MAX_THREAD_KEYS]; +} guac_thread_storage_t; Review Comment: Please document all structure members. ########## src/libguac/thread-local.c: ########## @@ -0,0 +1,203 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "config.h" +#include "guacamole/thread-local.h" + +#include <stdlib.h> +#include <pthread.h> +#include <errno.h> +#include <sched.h> + +/** + * Maximum number of thread-local keys supported. + */ +#define MAX_THREAD_KEYS 1024 Review Comment: If the issue is partly: > scalability limitations where systems are typically limited to 1024 pthread keys per process doesn't this default leave the same limitation in place by default? ########## src/libguac/thread-local.c: ########## @@ -0,0 +1,203 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "config.h" +#include "guacamole/thread-local.h" + +#include <stdlib.h> +#include <pthread.h> +#include <errno.h> +#include <sched.h> + +/** + * Maximum number of thread-local keys supported. + */ +#define MAX_THREAD_KEYS 1024 + +/** + * Structure to hold destructor information for each key. + */ +typedef struct guac_key_entry { + guac_thread_local_destructor_t destructor; + int in_use; +} guac_key_entry_t; + +/** + * Thread-local storage entry. + */ +typedef struct guac_thread_storage { + void* values[MAX_THREAD_KEYS]; +} guac_thread_storage_t; + +/** + * Global key registry protected by mutex. + */ +static guac_key_entry_t key_registry[MAX_THREAD_KEYS]; +static pthread_mutex_t key_registry_mutex = PTHREAD_MUTEX_INITIALIZER; +static uintptr_t next_key_id = 1; + +/** + * Thread-local storage using __thread keyword for better performance. + */ +static __thread guac_thread_storage_t* thread_storage = NULL; + +/** + * Global cleanup key created once for all threads. + */ +static pthread_key_t global_cleanup_key; +static pthread_once_t cleanup_key_once = PTHREAD_ONCE_INIT; + +/** + * Cleanup function called when thread exits. + */ +static void cleanup_thread_storage(void* storage) { + guac_thread_storage_t* ts = (guac_thread_storage_t*)storage; + if (ts == NULL) return; + + pthread_mutex_lock(&key_registry_mutex); + + for (int i = 0; i < MAX_THREAD_KEYS; i++) { + if (key_registry[i].in_use && ts->values[i] != NULL) { + if (key_registry[i].destructor) { + key_registry[i].destructor(ts->values[i]); + } + } + } + + pthread_mutex_unlock(&key_registry_mutex); + free(ts); +} + +/** + * Initialize the global cleanup key once. + */ +static void init_cleanup_key(void) { + pthread_key_create(&global_cleanup_key, cleanup_thread_storage); +} + +/** + * Initialize thread storage if not already done. + */ +static guac_thread_storage_t* get_thread_storage(void) { + if (thread_storage == NULL) { + thread_storage = calloc(1, sizeof(guac_thread_storage_t)); + if (thread_storage != NULL) { + pthread_once(&cleanup_key_once, init_cleanup_key); + if (pthread_setspecific(global_cleanup_key, thread_storage) != 0) { + /* Critical: if pthread_setspecific fails, we must not leave + * the thread_storage pointer set, as cleanup won't be called */ + free(thread_storage); + thread_storage = NULL; + return NULL; + } + } + } + return thread_storage; +} + +int guac_thread_local_key_create(guac_thread_local_key_t* key, guac_thread_local_destructor_t destructor) { + if (key == NULL) return EINVAL; + + pthread_mutex_lock(&key_registry_mutex); + + uintptr_t key_id = 0; + for (int i = 0; i < MAX_THREAD_KEYS; i++) { + if (!key_registry[i].in_use) { + key_id = next_key_id++; + if (next_key_id == 0) next_key_id = 1; // Avoid key_id of 0 + + key_registry[i].destructor = destructor; + key_registry[i].in_use = 1; + *key = (key_id << 16) | i; // Combine ID and index for validation + break; + } + } Review Comment: > Supports unlimited keys (configurable, default 1024) via hash-table implementation This looks more like a brute-force search of the full table of keys. Am I missing something? ########## src/libguac/tests/thread-local/thread_local_storage_multithreaded.c: ########## @@ -0,0 +1,129 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include <CUnit/CUnit.h> +#include <guacamole/thread-local.h> + +#include <stdlib.h> +#include <pthread.h> +#include <unistd.h> + +static guac_thread_local_key_t test_key; +static int destructor_call_count = 0; +static pthread_mutex_t destructor_mutex = PTHREAD_MUTEX_INITIALIZER; + +/** + * Destructor function for testing cleanup. + */ +static void test_destructor(void* value) { + pthread_mutex_lock(&destructor_mutex); + destructor_call_count++; + pthread_mutex_unlock(&destructor_mutex); + free(value); +} + +/** + * Thread worker function for multithreaded tests. + */ +static void* thread_worker(void* arg) { + int thread_id = *(int*)arg; + + /* Each thread sets its own value */ + int* test_value = malloc(sizeof(int)); + *test_value = thread_id * 100; + + CU_ASSERT_EQUAL(guac_thread_local_setspecific(test_key, test_value), 0); + + /* Small delay to increase chance of race conditions */ + usleep(1000); Review Comment: If all threads wait for the same duration at the same point, how does this increase the chance of a race condition? Wouldn't this be equivalent to waiting for any constant duration, including not waiting at all? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
