Skip to content
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

Fixed various threadpool errors #1

Merged
merged 3 commits into from
Dec 15, 2022
Merged

Fixed various threadpool errors #1

merged 3 commits into from
Dec 15, 2022

Conversation

sergio-nsk
Copy link
Collaborator

  • double decremented threadpool->num_threads
  • data race while writing threadpool->stop and reading in threads
  • attempt to unlock of unlocked mutex

@nmoinvaz
Copy link
Owner

nmoinvaz commented Dec 15, 2022

I think for the double-decrement of num_threads, that the decrement should be removed from threadpool_delete_threads instead of inside the thread. The other changes look good.

@sergio-nsk
Copy link
Collaborator Author

sergio-nsk commented Dec 15, 2022

I don't think the decrement in a thread function is a good idea by the several reasons:

  • threadpool_do_work can early exit if thread is NULL and num_threads will not be decremented.
  • pthread_create() can fail and num_threads will be kept incremented.
  • num_threads is a list length from the code logic, thus the decrement should be at the list manipulations.

@nmoinvaz
Copy link
Owner

nmoinvaz commented Dec 15, 2022

threadpool_do_work can early exit if thread is NULL and num_threads will not be decremented.

We can remove the check for if (!thread) since this will never be the case.

pthread_create() can fail and num_threads will be kept incremented.

We can just do:

    if (pthread_create(&thread->handle, NULL, threadpool_do_work, thread))
        threadpool->num_threads--;

num_threads is a list length from the code logic, thus the decrement should be at the list manipulations.

The problem with decrementing num_threads from threadpool_delete_threads instead of threadpool_do_work is that threadpool_delete_threads is just there to clean up all the threads. So it doesn't even make sense that I am decrementing there.

@nmoinvaz nmoinvaz changed the title Fixed various errors Fixed various threadpool errors Dec 15, 2022
threadpool_winxp.c Outdated Show resolved Hide resolved
@sergio-nsk sergio-nsk requested a review from nmoinvaz December 15, 2022 22:49
@nmoinvaz
Copy link
Owner

Looks like does not pass build.

@nmoinvaz nmoinvaz merged commit 713cb96 into nmoinvaz:master Dec 15, 2022
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