diff options
| author |  Martin Ågren <martin.agren@gmail.com> | 2017-08-21 19:43:46 +0200 |
|---|---|---|
| committer |  Junio C Hamano <gitster@pobox.com> | 2017-08-23 10:14:19 -0700 |
| commit | 0c2ad00b3c5a822224265c2551844b2eafc89875 (patch) | |
| tree | fb3e9dfbd527040cee900ac0ba744798a9b8a9c5 | |
| parent | convert: always initialize attr_action in convert_attrs (diff) | |
| download | tgif-0c2ad00b3c5a822224265c2551844b2eafc89875.tar.xz | |
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>
| -rw-r--r-- | builtin/pack-objects.c | 6 |
1 files changed, 6 insertions, 0 deletions
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index f4a8441fe9..ffb13f7800 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -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); @@ -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; } |