-
Notifications
You must be signed in to change notification settings - Fork 240
fix(thread-safety): eliminate races in log mutex, watcher, and index threads #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,15 @@ int cbm_thread_join(cbm_thread_t *t) { | |
| return CBM_NOT_FOUND; | ||
| } | ||
| CloseHandle(t->handle); | ||
| t->handle = NULL; | ||
| return 0; | ||
| } | ||
|
|
||
| int cbm_thread_detach(cbm_thread_t *t) { | ||
| if (t->handle) { | ||
| CloseHandle(t->handle); | ||
| t->handle = NULL; | ||
| } | ||
| return 0; | ||
| } | ||
|
Comment on lines
+63
to
+69
|
||
|
|
||
|
|
@@ -74,7 +83,19 @@ int cbm_thread_create(cbm_thread_t *t, size_t stack_size, void *(*fn)(void *), v | |
| } | ||
|
|
||
| int cbm_thread_join(cbm_thread_t *t) { | ||
| return pthread_join(t->handle, NULL); | ||
| int rc = pthread_join(t->handle, NULL); | ||
| if (rc == 0) { | ||
| memset(&t->handle, 0, sizeof(t->handle)); | ||
| } | ||
| return rc; | ||
| } | ||
|
|
||
| int cbm_thread_detach(cbm_thread_t *t) { | ||
| int rc = pthread_detach(t->handle); | ||
| if (rc == 0) { | ||
| memset(&t->handle, 0, sizeof(t->handle)); | ||
| } | ||
| return rc; | ||
| } | ||
|
|
||
| #endif | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |||||||||||||||
| #include "foundation/log.h" | ||||||||||||||||
| #include "foundation/hash_table.h" | ||||||||||||||||
| #include "foundation/compat.h" | ||||||||||||||||
| #include "foundation/compat_thread.h" | ||||||||||||||||
| #include "foundation/compat_fs.h" | ||||||||||||||||
| #include "foundation/str_util.h" | ||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -50,6 +51,7 @@ struct cbm_watcher { | |||||||||||||||
| cbm_index_fn index_fn; | ||||||||||||||||
| void *user_data; | ||||||||||||||||
| CBMHashTable *projects; /* name → project_state_t* */ | ||||||||||||||||
| cbm_mutex_t projects_lock; | ||||||||||||||||
| atomic_int stopped; | ||||||||||||||||
| }; | ||||||||||||||||
|
Comment on lines
51
to
56
|
||||||||||||||||
|
|
||||||||||||||||
|
|
@@ -236,6 +238,7 @@ cbm_watcher_t *cbm_watcher_new(cbm_store_t *store, cbm_index_fn index_fn, void * | |||||||||||||||
| w->index_fn = index_fn; | ||||||||||||||||
| w->user_data = user_data; | ||||||||||||||||
| w->projects = cbm_ht_create(CBM_SZ_32); | ||||||||||||||||
|
||||||||||||||||
| w->projects = cbm_ht_create(CBM_SZ_32); | |
| w->projects = cbm_ht_create(CBM_SZ_32); | |
| if (!w->projects) { | |
| free(w); | |
| return NULL; | |
| } |
Copilot
AI
Apr 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state_new() can return NULL on OOM, but the result is used immediately (s->project_name) and inserted into the hash table while holding projects_lock. Please handle allocation failure by unlocking, cleaning up, and returning (optionally logging an OOM warning).
| project_state_t *s = state_new(project_name, root_path); | |
| project_state_t *s = state_new(project_name, root_path); | |
| if (!s) { | |
| cbm_mutex_unlock(&w->projects_lock); | |
| cbm_log_warn("watcher.watch.oom", "project", project_name, "path", root_path); | |
| return; | |
| } |
Copilot
AI
Apr 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cbm_watcher_watch_count() takes a const cbm_watcher_t* but then casts away const to lock/unlock projects_lock. This undermines const-correctness and can surprise callers. Consider changing the API to take a non-const cbm_watcher_t* (since it necessarily touches synchronization primitives), or store projects_lock behind an internal pointer so the public signature remains const-safe.
Copilot
AI
Apr 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new coarse lock in cbm_watcher_poll_once() is held across the entire cbm_ht_foreach(...) call. If poll_project performs I/O, indexing work, or calls out to callbacks, this can block watch/unwatch for long periods and increase contention. A common pattern is to snapshot the current project states/keys under the lock (e.g., into a temporary array), release the lock, and then run poll_project over the snapshot to keep the critical section small.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Windows, cbm_thread_join closes the thread handle but does not clear t->handle. With cbm_thread_detach now available (and used elsewhere), a caller could accidentally CloseHandle twice (join then detach), or treat the thread as still valid. Set t->handle = NULL after CloseHandle in cbm_thread_join for consistency with cbm_thread_detach.