@matheusbsbs / matheus-bagliano / commits / 0c2ad00b3c

pack-objects: take lock before accessing `remaining`

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 <martin.agren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Martin Ågren committed Aug 21, 2017 at 19:43 UTC 0c2ad00b3c5a822224265c2551844b2eafc89875

1 file changed +6

builtin/pack-objects.c

@@ -2171,7 +2171,10 @@ static void *threaded_find_deltas(void *arg)

2171 {

2172 struct thread_params *me = arg;

2173

2174 + progress_lock();

2175 while (me->remaining) {

2176 + progress_unlock();

2177 +

2178 find_deltas(me->list, &me->remaining,

2179 me->window, me->depth, me->processed);

2180

@@ -2193,7 +2196,10 @@ static void *threaded_find_deltas(void *arg)

2196 pthread_cond_wait(&me->cond, &me->mutex);

2197 me->data_ready = 0;

2198 pthread_mutex_unlock(&me->mutex);

2199 +

2200 + progress_lock();

2201 }

2202 + progress_unlock();

2203 /* leave ->working 1 so that this doesn't get more work assigned */

2204 return NULL;

2205 }