Skip to content

threadpool: fix a race condition during downsizing #4129

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

Closed

Conversation

kleisauke
Copy link
Member

@kleisauke kleisauke commented Aug 29, 2024

Under certain conditions, particularly with short-lived pipelines and high thread concurrency, a race condition could occur between decrementing and restoring the exit flag atomically, potentially causing more threads to cooperatively exit than intended.

Fix this race by using a single atomic compare-and-exchange, which only resets the flag when it is set. Additionally, the flag's type has been changed from int to gboolean for clarity.

Context: #4069.

kleisauke added a commit to libvips/build-win64-mxe that referenced this pull request Aug 29, 2024
Under certain conditions, particularly with short-lived pipelines
and high thread concurrency, a race condition could occur between
decrementing and restoring the `exit` flag atomically, potentially
causing more threads to cooperatively exit than intended.

Fix this race by using a single atomic compare-and-exchange, which
only resets the flag when it is set. Additionally, the flag's type
has been changed from `int` to `gboolean` for clarity.
@kleisauke kleisauke force-pushed the 8.15-threadpool-fix-downsize-race branch from 446593d to 4c1de0f Compare August 29, 2024 18:02
@kleisauke
Copy link
Member Author

Upon a closer look, I don't think this is a race condition. The timing of the exit count increment in vips_threadpool_run() likely doesn't matter, even if it occurs between the decrement and increment of the exit count in vips_worker_work_unit().

I do like the g_atomic_int_compare_and_exchange() and gboolean simplification though, so perhaps this PR should turn into a polishing PR. However, I'm uncertain whether it should target the 8.15 branch in that case.

@jcupitt
Copy link
Member

jcupitt commented Sep 1, 2024

Good idea, let's make this a polish on master instead.

@kleisauke
Copy link
Member Author

Great! Closing in favor of PR #4123.

@kleisauke kleisauke closed this Sep 1, 2024
@kleisauke kleisauke deleted the 8.15-threadpool-fix-downsize-race branch September 1, 2024 13:24
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.

2 participants