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

race condition in workpool #70

Open
pmsoftware78 opened this issue Jan 21, 2020 · 2 comments
Open

race condition in workpool #70

pmsoftware78 opened this issue Jan 21, 2020 · 2 comments

Comments

@pmsoftware78
Copy link

Hi All,
I suggest to wait until all threads completed startup before returning in workerpool_create function, adding

        pthread_mutex_lock(&wp->mutex);

        while (wp->end_count < wp->nthreads) {
            pthread_cond_wait(&wp->endcond, &wp->mutex);
        }
        
        pthread_mutex_unlock(&wp->mutex);

and in workerpool_run function, wp->end_count editing should be protected (ok... this problem should not happen), inserting it after mutex lock:

        pthread_mutex_lock(&wp->mutex);
        wp->end_count = 0; 
        pthread_cond_broadcast(&wp->startcond);

Best
Paolo

@pmsoftware78 pmsoftware78 changed the title Crititical Run in workpool Critical Run in workpool Jan 21, 2020
@pmsoftware78 pmsoftware78 changed the title Critical Run in workpool race condition in workpool Jan 21, 2020
@mkrogius
Copy link
Contributor

Hi Paolo, I'm not sure I see exactly what the problem is with the current code. What race condition are you seeing?

@pmsoftware78
Copy link
Author

Hi Max,
urgh... so time ago! I try to remember all the details....
during workerpool_create all threads are created using pthread_create but not all threads at the end of this function can be started and/or not all threads are blocked on pthread_cond_wait, so this part of the code in worker_thread

pthread_mutex_lock(&wp->mutex);
        while (wp->taskspos == zarray_size(wp->tasks)) {
            wp->end_count++;  //< this 

and

void workerpool_run(workerpool_t *wp)
{
    if (wp->nthreads > 1) {
        wp->end_count = 0; //< this

could be execute in wrong order and wp->end_count will have wrong value.

I try to understand what you want to do and my idea is to wait that all threads be in a wait state

  pthread_mutex_lock(&wp->mutex);

        while (wp->end_count < wp->nthreads) {
            pthread_cond_wait(&wp->endcond, &wp->mutex);
        }
        
        pthread_mutex_unlock(&wp->mutex);

at the end of create and for extra security lock endcound access

// runs all added tasks, waits for them to complete.
void workerpool_run(workerpool_t *wp)
{
    if (wp->nthreads > 1) {

        pthread_mutex_lock(&wp->mutex);
        wp->end_count = 0; // FIX
        pthread_cond_broadcast(&wp->startcond);

        while (wp->end_count < wp->nthreads) {
//            printf("caught %d\n", wp->end_count);
            pthread_cond_wait(&wp->endcond, &wp->mutex);
        }

        pthread_mutex_unlock(&wp->mutex);

        wp->taskspos = 0;

        zarray_clear(wp->tasks);

    } else {
        workerpool_run_single(wp);
    }
}

I hope I have been clear enough.

Best
Paolo

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

No branches or pull requests

2 participants