Work around race condition in subprocess.

Work around race condition in subprocess.

There's a race condition in python's subprocess module, and gclient uses
it heavily while multithreaded. Avoid the race by locking around calls
to subprocess.Popen's constructor. Detailed explanation in the bug.

BUG=531561
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=296685

[Torne, 2015-09-14 16:30:27]

torne@chromium.org changed reviewers:
+ iannucci@chromium.org

[Torne, 2015-09-14 16:30:27]

Here's a terrifying one! Hope you're a reasonable person to review?

I tested this with the config mentioned in the bug (git-remote-persistent-https)
and was able to repro gclient hanging about 90% of the time before, but can't
make it hang after.

[iannucci, 2015-09-14 17:03:02]

iannucci@chromium.org changed reviewers:
+ maruel@chromium.org, pgervais@chromium.org

[iannucci, 2015-09-14 17:03:03]

Oh wow! Maruel, pgervais, we probably have this bug in other places. I'll review
on detail on a real computer, but it probably can't make things worse.

[M-A Ruel, 2015-09-14 17:07:20]

On 2015/09/14 17:03:03, iannucci wrote:
> Oh wow! Maruel, pgervais, we probably have this bug in other places. I'll
review
> on detail on a real computer, but it probably can't make things worse.

#python

Ugh, thanks for the diagnosis. I haven't confirmed with the code yet but that
looks really bad. Do you know if there's an upstream bug and if it was fixed?

I'll also have to fix it in subprocess42.py, which will be trivial there.

[iannucci, 2015-09-14 17:08:36]

I sent a blurb to c-i-t. Hopefully we can stamp it out in other places.

On Mon, Sep 14, 2015, 10:07  <maruel@chromium.org> wrote:

> On 2015/09/14 17:03:03, iannucci wrote:
> > Oh wow! Maruel, pgervais, we probably have this bug in other places. I'll
> review
> > on detail on a real computer, but it probably can't make things worse.
>
> #python
>
> Ugh, thanks for the diagnosis. I haven't confirmed with the code yet but
> that
> looks really bad. Do you know if there's an upstream bug and if it was
> fixed?
>
> I'll also have to fix it in subprocess42.py, which will be trivial there.
>
> https://codereview.chromium.org/1343783004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Torne, 2015-09-14 17:22:08]

On 2015/09/14 17:07:20, M-A Ruel wrote:
> On 2015/09/14 17:03:03, iannucci wrote:
> > Oh wow! Maruel, pgervais, we probably have this bug in other places. I'll
> review
> > on detail on a real computer, but it probably can't make things worse.
> 
> #python
> 
> Ugh, thanks for the diagnosis. I haven't confirmed with the code yet but that
> looks really bad. Do you know if there's an upstream bug and if it was fixed?

No idea. There's a bunch of upstream bugs related to subprocess and threads, I
haven't read enough of them to see if this is one of them :)

> I'll also have to fix it in subprocess42.py, which will be trivial there.

There are some places in gclient where we don't use subprocess2, also. I haven't
done anything about them here.

[M-A Ruel, 2015-09-14 20:15:12]

This CL lgtm, the rest can be looked at later.

[Torne, 2015-09-15 09:54:43]

The CQ bit was checked by torne@chromium.org

[commit-bot: I haz the power, 2015-09-15 09:54:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1343783004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1343783004/1

[commit-bot: I haz the power, 2015-09-15 09:57:03]

Committed patchset #1 (id:1) as
http://src.chromium.org/viewvc/chrome?view=rev&revision=296685