Re: [PATCH] Work around performance bug in subprocess.Popen.communicate()

From: Karl Wiberg <kha@virtutech.com>
Date: Aug 14, 2009, 06:21 UTC
Message-ID: <20090814062149.GA23179@lux.e.vtech>
In-Reply-To: <b0943d9e0908131518i3ac18331leb4c0c76313b0780@mail.gmail.com>

On 2009-08-13 23:18:59 +0100, Catalin Marinas wrote:

Show 24 quoted lines

> 2009/8/4 Karl Wiberg <kha@virtutech.com>:
>
> > On 2009-07-31 12:27:53 +0100, Catalin Marinas wrote:
> >
> > > But can this not lead to a deadlock if the __indata is big? The
> > > stdout of the created process is only processed once the whole
> > > __indata is written. I thought communicate() was created to
> > > avoid this issue.
> >
> > I don't think there's a problem. write() isn't supposed to have a
> > limit on the amount of data it will accept in one call, as far as
> > I'm aware. Plus, it works just fine with Erik's test case---which
> > in my case was about 7 MB. If it can handle 7 MB, I doubt there's
> > a limit we'll hit anytime soon.
>
> write() itself doesn't have a limit, it's mainly what the
> application receiving the data can handle. In the Git case, I think
> it takes all the input as it isn't a filtering tool (things may be
> different with tools like sed etc.).
>
> > Oh, and we still call communicate()---we just don't pass it any
> > additional bytes to write to stdin.
>
> Yes, but if write() is blocked, communicate() won't be called.

Ah, so that's what you were worrying about. Yeah, if the process we're writing to was going to block us (== not read input) for long periods of time, we'd have to handle that case. And if it was going to decide to stop reading input half-way through and give us a sigpipe, we'd have to handle that. But ...

> Since we are only using Git, I'll merge this patch

Exactly. We really don't expect git to do any of those things to us. We know it reads all available input before producing the output. (Well, almost always. I have patches in my experimental branch that use git cat-file --batch and git diff-tree --stdin, where there can be many rounds of talking back and forth, but when driving those I'm being very careful to not deadlock.)

> (and maybe add a comment).

Sure.

-- 
Karl Wiberg, Virtutech