2247: Deadlock when calling 'patch' with a very large diff

degrand********@gmai***** (Google Code) (Is this you? Claim this profile.)
david
david
Feb. 4, 2014
What version are you running?

1.6RC2

We often have a high memory consumption on our reviewboard server, and when it happens, there are some 'patch' processes blocked.

I looked at how you launch the 'patch' utility, and I think that there is a potential pb, due to the use of Popen.wait() with pipes. According to the documentation (http://docs.python.org/library/subprocess.html#popen-objects), there is a potential deadlock if the date sent to the pipe is larger than the pipe buffer size (we have some very huge diffs).

Here is a small python code to reproduce it:

--------------------------------------
import sys, subprocess

def main():

  data = ""
  for i in range(100000):
    data += "%d\n" % i

  p = subprocess.Popen(['cat'], stdin=subprocess.PIPE, stdout=subprocess.PIPE)
  p.stdin.write(data)
  p.stdin.close()
  output = p.stdout.read()
  failure = p.wait()

  if failure:
    print("error")
    sys.exit(1)

  sys.stdout.write(output)

if __name__ == '__main__': main()
-----------------------------------------------------------

As long as 'data' is small enough (i.e. with a small maximum value of 'i'), everything works well. But with 'i' ranging from 0 to 100000, p.wait() blocks.

The documentation recommends to use Popen.communicate()
#1 bas****@bran***** (Google Code) (Is this you? Claim this profile.)
We had the problem too since version 1.0.8. It would be nice if this is getting fixed to include the fix in version 1.5. * as well. 
chipx86
#2 chipx86
I can see what we can do. I have noticed this as well. We're very close to release and, given that it's been a problem for a while, it may end up slipping to the next point release if we can't come up with a good, testable solution right away.
  • +Confirmed
  • -Priority-Medium
    +Priority-High
    +Milestone-Release1.6
    +Component-DiffViewer
chipx86
#3 chipx86
There doesn't look to be a simple solution, and the ones I've seen for this are platform-dependent. Since it's not a regression in 1.6, I'm pushing it out to 1.6.x until we can figure out a way to fix it.
  • -Milestone-Release1.6
    +Milestone-Release1.6.x
david
#4 david
  • -Milestone-Release1.6.x
david
#5 david
  • -Confirmed
    +PendingReview
  • +david
david
#6 david
Fixed in master (6c0870d). Thanks!
  • -PendingReview
    +Fixed