-
Notifications
You must be signed in to change notification settings - Fork 143
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 all commits
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 | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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.
The POSIX implementation doesn’t clear/poison the stored
pthread_tafter a successful detach, which makes accidental laterjoin()/double-detach harder to detect and can lead to inconsistent lifecycle handling across platforms (Windows clears the handle). Consider, on success, updating the thread object to a known 'no handle' state (whatever is idiomatic forcbm_thread_ton POSIX), and/or documenting thatcbm_thread_join()must not be called after detach.