Merge lp:~spiv/bzr/import-tariff-test-subprocess-deadlock into lp:bzr

[Andrew Bennetts]

In theory this branch should fix the deadlock in test_import_tariff on the Windows buildslave in babune. The theory is that the stderr pipe has a finite buffer size, and we don't read from it until the process finishes, so if there's enough output the subprocess will hang and so will our test. (stdout and stdin don't need any special attention here because they are managed by the smart server protocol code, so they should be fine because the smart protocol code always knows when it can read from stdout.)

It would be good to get confirmation from Vincent that does work before landing it; I have a hazy post-sprint memory that perhaps he already did and it failed. If it doesn't we'll need to dig deeper to find out why it's still hanging.

[Vincent Ladeuil (vila)]

I will retry it but it wasn't addressing the problem on OSX (easier testing there for me).

Also note http://pad.lv/625551 which shares some bad smells with this issue.

And also http://pad.lv/788235 which also involves bzr_serve and hangs in a maverick chroot (easier to reproduce, several tests currently repeatedly fail (with some variants regarding which ones fail)).

I think all of them are revealing a racing condition we never encounter before but was always present.

Oh, and of course the breakin tests which have now been deleted by poolie were also probably hitting the same problem but were blacklisted on babune.

This bug had fun with us for too long, I officially call for a war against it ;)

[Vincent Ladeuil (vila)]

Confirmed, still hanging on OSX :-/

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/27/2011 09:48 AM, Vincent Ladeuil wrote:
> Confirmed, still hanging on OSX :-/

Can we use a temporary file instead of a file in the current directory?
Otherwise I think this is better than before, regardless of whether it
makes the specific bits better.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3fXRQACgkQJdeBCYSNAAPOZACfVtO109OSFV56o9yewsYU4U/v
fgYAoNP766/neOvk1Yme9n+wCImqNTWE
=awSA
-----END PGP SIGNATURE-----

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/27/2011 10:15 AM, John A Meinel wrote:
> On 05/27/2011 09:48 AM, Vincent Ladeuil wrote:
>> Confirmed, still hanging on OSX :-/
>
> Can we use a temporary file instead of a file in the current directory?
> Otherwise I think this is better than before, regardless of whether it
> makes the specific bits better.
>
> John
> =:->
>

I take that back. I think the test is already isolated in a temp dir,
just using a local file is fine.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk3fXfoACgkQJdeBCYSNAAMEswCdE2bXxwtUiw5OUt3W270Ghskx
pEYAn1aVf4WNQZBKqPhUCA4cY3NO7lnL
=jwFN
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

This proposal is itself in a deadlock state :)

It doesn't address the bug but we still don't know why.

Marking it as WIP until we know better and it's resubmitted or we found a different approach and it's abandoned.

review: Needs Fixing

[Vincent Ladeuil (vila)]

Rejoice !

The plan was good ! The implementation... missed a tiny tiny little bit :)

I even wonder what happened there, I can only suspect you tested it successfully on the failing test, tried all import tests, realized one was broken, fixed it... without realizing you broke the first one ;)

But anyway, well done !

The missing tiny little bit is at lp:~vila/bzr/import-tariff-test-subprocess-deadlock which I will land, because that will be a nice way to end the week !

The war is over ! This bug is dead !

=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2011-05-26 08:05:45 +0000
+++ bzrlib/tests/__init__.py	2011-05-27 05:54:44 +0000
@@ -2013,7 +2013,7 @@
     def start_bzr_subprocess(self, process_args, env_changes=None,
                              skip_if_plan_to_signal=False,
                              working_dir=None,
-                             allow_plugins=False):
+                             allow_plugins=False, stderr=subprocess.PIPE):
         """Start bzr in a subprocess for testing.
 
         This starts a new Python interpreter and runs bzr in there.
@@ -2031,6 +2031,9 @@
         :param skip_if_plan_to_signal: raise TestSkipped when true and system
             doesn't support signalling subprocesses.
         :param allow_plugins: If False (default) pass --no-plugins to bzr.
+        :param stderr: file to use for the subprocess's stderr.  Valid values
+            are those valid for the stderr argument of `subprocess.Popen`.
+            Default value is ``subprocess.PIPE``.
 
         :returns: Popen object for the started process.
         """
@@ -2071,7 +2074,7 @@
             command.extend(process_args)
             process = self._popen(command, stdin=subprocess.PIPE,
                                   stdout=subprocess.PIPE,
-                                  stderr=subprocess.PIPE)
+                                  stderr=stderr)
         finally:
             restore_environment()
             if cwd is not None:

=== modified file 'bzrlib/tests/test_import_tariff.py'
--- bzrlib/tests/test_import_tariff.py	2011-05-03 23:16:56 +0000
+++ bzrlib/tests/test_import_tariff.py	2011-05-27 05:54:44 +0000
@@ -62,7 +62,7 @@
             self.preserved_env_vars[name] = os.environ.get(name)
         super(TestImportTariffs, self).setUp()
 
-    def start_bzr_subprocess_with_import_check(self, args):
+    def start_bzr_subprocess_with_import_check(self, args, stderr_file=None):
         """Run a bzr process and capture the imports.
 
         This is fairly expensive because we start a subprocess, so we aim to
@@ -187,8 +187,11 @@
     def test_simple_serve(self):
         # 'serve' in a default format working tree shouldn't need many modules
         tree = self.make_branch_and_tree('.')
+        # Capture the bzr serve process' stderr in a file to avoid deadlocks
+        # while the smart client interacts with it.
+        stderr_file = open('bzr-serve.stderr', 'w')
         process = self.start_bzr_subprocess_with_import_check(['serve',
-            '--inet', '-d', tree.basedir])
+            '--inet', '-d', tree.basedir], stderr_file=stderr_file)
         url = 'bzr://localhost/'
         self.permit_url(url)
         client_medium = medium.SmartSimplePipesClientMedium(
@@ -200,6 +203,9 @@
         process.stdin = None
         (out, err) = self.finish_bzr_subprocess(process,
             universal_newlines=False)
+        stderr_file.close()
+        with open('bzr-serve.stderr', 'r') as stderr_file:
+            err = stderr_file.read()
         self.check_forbidden_modules(err,
             ['bzrlib.annotate',
             'bzrlib.atomicfile',

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-05-26 20:24:20 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-05-27 05:54:44 +0000
@@ -59,6 +59,12 @@
    suite.  This can include new facilities for writing tests, fixes to 
    spurious test failures and changes to the way things should be tested.
 
+* Fix deadlock in `TestImportTariffs.test_simple_serve` when stderr gets
+  more output than fits in the default buffer.  This was happening on the
+  Windows buildslave, and could easily happen in other circumstances where
+  the default OS buffer size for pipes is small or the ``python -v``
+  output is large.  (Andrew Bennetts, #784802)
+
 
 bzr 2.4b3
 #########