Skip to content

Conversation

@veloman-yunkan
Copy link
Collaborator

@veloman-yunkan veloman-yunkan commented May 4, 2025

Fixes #983

@codecov
Copy link

codecov bot commented May 4, 2025

Codecov Report

❌ Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.65%. Comparing base (5b80090) to head (c86c695).
⚠️ Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
src/namedthread.h 25.00% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #984      +/-   ##
==========================================
- Coverage   57.65%   57.65%   -0.01%     
==========================================
  Files         103      103              
  Lines        4922     4926       +4     
  Branches     2066     2068       +2     
==========================================
+ Hits         2838     2840       +2     
  Misses        706      706              
- Partials     1378     1380       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Before this fix, `NamedThread::getCurrentThreadName()` might be called
from the new thread (that already started executing) before the
assignment to `thread_` in the `NamedThread` constructor had taken
place. Then the query to the thread id (`nt->thread_.get_id()`) on such
a thread did not return the correct id, and the end result was that a
new (numbered) thread name was synthesized for that thread. From that
point onward, concurrency orchestration via `log_debug()` (which relies
on correct thread names) broke, typically resulting in a deadlock.

This fix slightly reduces the generality of the `NamedThread` constructor
(support for arguments to the callable has been dropped) since it is not
currently needed (and even if it was needed, it could be circumvented
with an extra lambda expression).
@kelson42 kelson42 force-pushed the namedthread_bugfix branch from eb66991 to 3ef0774 Compare May 4, 2025 08:07
@kelson42 kelson42 added this to the 9.4.0 milestone May 4, 2025
@kelson42
Copy link
Contributor

kelson42 commented May 4, 2025

@veloman-yunkan This PR seems to introduce a compilation regression (Windows only!), see https://github.com/openzim/libzim/actions/runs/14819235224/job/41603865419?pr=984

@kelson42 kelson42 requested review from kelson42 and removed request for mgautierfr May 4, 2025 10:46
@veloman-yunkan veloman-yunkan merged commit 30bac7d into main May 4, 2025
30 of 31 checks passed
@veloman-yunkan veloman-yunkan deleted the namedthread_bugfix branch May 4, 2025 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Race condition in NamedThread

3 participants