[PATCH] revset: fix generatorset race condition

Gregory Szorc gregory.szorc at gmail.com
Tue Mar 25 19:24:46 CDT 2014


On 3/25/14, 5:13 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham at fb.com>
> # Date 1395789007 25200
> #      Tue Mar 25 16:10:07 2014 -0700
> # Node ID 719d5d962b0c8630c153453a48bfaeccbe9de2f7
> # Parent  9a09a625bc93ab69248613abcbea041b785f1a0e
> revset: fix generatorset race condition
>
> If two things were iterating over a generatorset at the same time, they could
> miss out on the things the other was generating, resulting in incomplete
> results. This fixes it by making it possible for two things to iterate at once,
> by always checking the _genlist at the beginning of each iteration.
>
> I was only able to repro it with pending changes from my other commits, but they
> aren't ready yet. So I'm unable to add a test for now.
>
> diff --git a/mercurial/revset.py b/mercurial/revset.py
> --- a/mercurial/revset.py
> +++ b/mercurial/revset.py
> @@ -2624,10 +2624,8 @@
>           gen: a generator producing the values for the generatorset.
>           """
>           self._gen = gen
> -        self._iter = iter(gen)
>           self._cache = {}
>           self._genlist = baseset([])
> -        self._iterated = False
>           self._finished = False
>
>       def __contains__(self, x):
> @@ -2639,28 +2637,29 @@
>               if l == x:
>                   return True
>
> -        self._finished = True
>           self._cache[x] = False
>           return False
>
>       def __iter__(self):
> -        if self._iterated:
> -            # At least a part of the list should be cached if iteration has
> -            # started over the generatorset.
> -            for l in self._genlist:
> -                yield l
> -
> -        for item in self._consumegen():
> -            yield item
> +        if self._finished:
> +            for x in self._genlist:
> +                yield x

Shouldn't we return here? Otherwise, don't we iterate over self._genlist 
twice if self._finished == True?

Why do we even need this block? Won't the while loop below handle it?

> +
> +        i = 0
> +        genlist = self._genlist
> +        consume = self._consumegen()
> +        while True:
> +            if i < len(genlist):
> +                yield genlist[i]
> +            else:
> +                yield consume.next()
> +            i += 1
>
>       def _consumegen(self):
> -        self._iterated = True
> -
>           for item in self._gen:
>               self._cache[item] = True
>               self._genlist.append(item)
>               yield item
> -
>           self._finished = True
>
>       def set(self):
>



More information about the Mercurial-devel mailing list