Skip to content

Commit

Permalink
pack-objects: take lock before accessing remaining
Browse files Browse the repository at this point in the history
When checking the conditional of "while (me->remaining)", we did not
hold the lock. Calling find_deltas would still be safe, since it checks
"remaining" (after taking the lock) and is able to handle all values. In
fact, this could (currently) not trigger any bug: a bug could happen if
`remaining` transitioning from zero to non-zero races with the evaluation
of the while-condition, but these are always separated by the
data_ready-mechanism.

Make sure we have the lock when we read `remaining`. This does mean we
release it just so that find_deltas can take it immediately again. We
could tweak the contract so that the lock should be taken before calling
find_deltas, but let's defer that until someone can actually show that
"unlock+lock" has a measurable negative impact.

Signed-off-by: Martin Ågren <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>
  • Loading branch information
Martin Ågren authored and gitster committed Aug 23, 2017
1 parent 5c94c93 commit 0c2ad00
Showing 1 changed file with 6 additions and 0 deletions.
6 changes: 6 additions & 0 deletions builtin/pack-objects.c
Original file line number Diff line number Diff line change
Expand Up @@ -2171,7 +2171,10 @@ static void *threaded_find_deltas(void *arg)
{
struct thread_params *me = arg;

progress_lock();
while (me->remaining) {
progress_unlock();

find_deltas(me->list, &me->remaining,
me->window, me->depth, me->processed);

Expand All @@ -2193,7 +2196,10 @@ static void *threaded_find_deltas(void *arg)
pthread_cond_wait(&me->cond, &me->mutex);
me->data_ready = 0;
pthread_mutex_unlock(&me->mutex);

progress_lock();
}
progress_unlock();
/* leave ->working 1 so that this doesn't get more work assigned */
return NULL;
}
Expand Down

0 comments on commit 0c2ad00

Please sign in to comment.