fix issue96 - make capturing more resilient against KeyboardInterrupt

--HG--
branch : trunk

CHANGELOG

@@ -1,6 +1,11 @@
 Changes between 1.3.0 and 1.3.1
 ==================================================
 
+- fix issue96: make capturing more resilient against Control-C 
+  interruptions (involved somewhat substantial refactoring
+  to the underlying capturing functionality to avoid race 
+  conditions).
+
 - make py.test.cmdline.main() return the exitstatus 
   instead of raising (which is still done by py.cmdline.pytest())
   and make it so that py.test.cmdline.main() can be called

py/_io/capture.py

@@ -29,21 +29,25 @@ except ImportError:
 class FDCapture: 
     """ Capture IO to/from a given os-level filedescriptor. """
     
-    def __init__(self, targetfd, tmpfile=None): 
+    def __init__(self, targetfd, tmpfile=None, now=True):
         """ save targetfd descriptor, and open a new 
             temporary file there.  If no tmpfile is 
             specified a tempfile.Tempfile() will be opened
             in text mode. 
         """
         self.targetfd = targetfd
+        self._patched = []
         if tmpfile is None: 
             f = tempfile.TemporaryFile('wb+')
             tmpfile = dupfile(f, encoding="UTF-8") 
             f.close()
         self.tmpfile = tmpfile 
-        self._savefd = os.dup(targetfd)
+        if now:
-        os.dup2(self.tmpfile.fileno(), targetfd) 
+            self.start()
-        self._patched = []
+
+    def start(self):
+        self._savefd = os.dup(self.targetfd)
+        os.dup2(self.tmpfile.fileno(), self.targetfd) 
 
     def setasfile(self, name, module=sys): 
         """ patch <module>.<name> to self.tmpfile
@@ -62,10 +66,13 @@ class FDCapture:
     def done(self): 
         """ unpatch and clean up, returns the self.tmpfile (file object)
         """
-        os.dup2(self._savefd, self.targetfd) 
+        try:
+            os.dup2(self._savefd, self.targetfd) 
+            os.close(self._savefd) 
+            self.tmpfile.seek(0)
+        except (AttributeError, ValueError, OSError):
+            pass
         self.unsetfiles() 
-        os.close(self._savefd) 
-        self.tmpfile.seek(0)
         return self.tmpfile 
 
     def writeorg(self, data):
@@ -146,89 +153,92 @@ class Capture(object):
 
     def reset(self):
         """ reset sys.stdout/stderr and return captured output as strings. """
-        if hasattr(self, '_suspended'):
+        outfile, errfile = self.done() 
-            outfile = self._kwargs['out']
-            errfile = self._kwargs['err']
-            del self._kwargs
-        else:
-            outfile, errfile = self.done() 
         out, err = "", ""
-        if outfile:
+        if outfile and not outfile.closed:
             out = outfile.read()
             outfile.close()
-        if errfile and errfile != outfile:
+        if errfile and errfile != outfile and not errfile.closed:
             err = errfile.read()
             errfile.close()
         return out, err
 
     def suspend(self):
         """ return current snapshot captures, memorize tempfiles. """
-        assert not hasattr(self, '_suspended')
-        self._suspended = True
         outerr = self.readouterr()
         outfile, errfile = self.done()
-        self._kwargs['out'] = outfile
-        self._kwargs['err'] = errfile
         return outerr
 
-    def resume(self):
-        """ resume capturing with original temp files. """
-        assert self._suspended
-        self._initialize(**self._kwargs)
-        del self._suspended
-
 
 class StdCaptureFD(Capture): 
     """ This class allows to capture writes to FD1 and FD2 
         and may connect a NULL file to FD0 (and prevent
         reads from sys.stdin)
     """
-    def __init__(self, out=True, err=True, 
+    def __init__(self, out=True, err=True, mixed=False, 
-                 mixed=False, in_=True, patchsys=True): 
+        in_=True, patchsys=True, now=True):
-        self._kwargs = locals().copy()
+        self.in_ = in_
-        del self._kwargs['self']
-        self._initialize(**self._kwargs)
-
-    def _initialize(self, out=True, err=True, 
-                    mixed=False, in_=True, patchsys=True): 
         if in_:
             self._oldin = (sys.stdin, os.dup(0))
-            sys.stdin  = DontReadFromInput()
-            fd = os.open(devnullpath, os.O_RDONLY)
-            os.dup2(fd, 0)
-            os.close(fd)
         if out:
             tmpfile = None
             if hasattr(out, 'write'):
-                tmpfile = out
+                tmpfile = None
-            self.out = py.io.FDCapture(1, tmpfile=tmpfile)
+            self.out = py.io.FDCapture(1, tmpfile=tmpfile, now=False)
-            if patchsys: 
+            self.out_tmpfile = tmpfile
-                self.out.setasfile('stdout')
         if err:
-            if mixed and out:
+            if out and mixed:
                 tmpfile = self.out.tmpfile 
             elif hasattr(err, 'write'):
                 tmpfile = err
             else:
                 tmpfile = None
-            self.err = py.io.FDCapture(2, tmpfile=tmpfile) 
+            self.err = py.io.FDCapture(2, tmpfile=tmpfile, now=False) 
-            if patchsys: 
+            self.err_tmpfile = tmpfile
-                self.err.setasfile('stderr')
+        self.patchsys = patchsys
+        if now:
+            self.startall()
+
+    def startall(self):
+        if self.in_:
+            sys.stdin  = DontReadFromInput()
+            fd = os.open(devnullpath, os.O_RDONLY)
+            os.dup2(fd, 0)
+            os.close(fd)
+       
+        out = getattr(self, 'out', None) 
+        if out:
+            out.start()
+            if self.patchsys:
+                out.setasfile('stdout')
+        err = getattr(self, 'err', None)
+        if err:
+            err.start()
+            if self.patchsys:
+                err.setasfile('stderr')
+
+    def resume(self):
+        """ resume capturing with original temp files. """
+        #if hasattr(self, 'out'):
+        #    self.out.restart()
+        #if hasattr(self, 'err'):
+        #    self.err.restart()
+        self.startall()
 
     def done(self):
         """ return (outfile, errfile) and stop capturing. """
+        outfile = errfile = None
         if hasattr(self, 'out'): 
             outfile = self.out.done() 
-        else:
-            outfile = None
         if hasattr(self, 'err'): 
             errfile = self.err.done() 
-        else:
-            errfile = None 
         if hasattr(self, '_oldin'):
             oldsys, oldfd = self._oldin 
-            os.dup2(oldfd, 0)
+            try:
-            os.close(oldfd)
+                os.dup2(oldfd, 0)
+                os.close(oldfd)
+            except OSError:
+                pass
             sys.stdin = oldsys 
         return outfile, errfile 
 
@@ -252,69 +262,61 @@ class StdCapture(Capture):
         modifies sys.stdout|stderr|stdin attributes and does not 
         touch underlying File Descriptors (use StdCaptureFD for that). 
     """
-    def __init__(self, out=True, err=True, in_=True, mixed=False):
+    def __init__(self, out=True, err=True, in_=True, mixed=False, now=True):
-        self._kwargs = locals().copy()
+        self._oldout = sys.stdout
-        del self._kwargs['self']
+        self._olderr = sys.stderr
-        self._initialize(**self._kwargs)
+        self._oldin  = sys.stdin
-
+        if out and not hasattr(out, 'file'):
-    def _initialize(self, out, err, in_, mixed):
+            out = TextIO()
-        self._out = out
+        self.out = out
-        self._err = err 
-        self._in = in_
-        if out: 
-            self._oldout = sys.stdout
-            if not hasattr(out, 'write'):
-                out = TextIO()
-            sys.stdout = self.out = out
         if err:
-            self._olderr = sys.stderr
+            if mixed:
-            if out and mixed: 
+                err = out
-                err = self.out 
             elif not hasattr(err, 'write'):
                 err = TextIO()
-            sys.stderr = self.err = err
+        self.err = err
-        if in_:
+        self.in_ = in_
-            self._oldin  = sys.stdin
+        if now:
-            sys.stdin  = self.newin  = DontReadFromInput()
+            self.startall()
+
+    def startall(self):
+        if self.out: 
+            sys.stdout = self.out
+        if self.err: 
+            sys.stderr = self.err
+        if self.in_:
+            sys.stdin  = self.in_  = DontReadFromInput()
 
     def done(self): 
         """ return (outfile, errfile) and stop capturing. """
-        o,e = sys.stdout, sys.stderr
+        outfile = errfile = None
-        if self._out: 
+        if self.out and not self.out.closed:
-            try:
+            sys.stdout = self._oldout 
-                sys.stdout = self._oldout 
-            except AttributeError:
-                raise IOError("stdout capturing already reset")
-            del self._oldout
             outfile = self.out
             outfile.seek(0)
-        else:
+        if self.err and not self.err.closed: 
-            outfile = None
+            sys.stderr = self._olderr 
-        if self._err: 
-            try:
-                sys.stderr = self._olderr 
-            except AttributeError:
-                raise IOError("stderr capturing already reset")
-            del self._olderr 
             errfile = self.err 
             errfile.seek(0)
-        else:
+        if self.in_:
-            errfile = None
-        if self._in:
             sys.stdin = self._oldin 
         return outfile, errfile
 
+    def resume(self):
+        """ resume capturing with original temp files. """
+        self.startall()
+
     def readouterr(self):
         """ return snapshot value of stdout/stderr capturings. """
         out = err = ""
-        if self._out:
+        if self.out:
-            out = sys.stdout.getvalue()
+            out = self.out.getvalue()
-            sys.stdout.truncate(0)
+            self.out.truncate(0)
-            sys.stdout.seek(0)
+            self.out.seek(0)
-        if self._err:
+        if self.err:
-            err = sys.stderr.getvalue()
+            err = self.err.getvalue()
-            sys.stderr.truncate(0)
+            self.err.truncate(0)
-            sys.stderr.seek(0)
+            self.err.seek(0)
         return out, err 
 
 class DontReadFromInput:
@@ -344,5 +346,3 @@ except AttributeError:
         devnullpath = 'NUL'
     else:
         devnullpath = '/dev/null'
-
-

py/_plugin/pytest_capture.py

@@ -106,6 +106,14 @@ def addouterr(rep, outerr):
 def pytest_configure(config):
     config.pluginmanager.register(CaptureManager(), 'capturemanager')
 
+class NoCapture:
+    def startall(self):
+        pass
+    def resume(self):
+        pass
+    def suspend(self):
+        return "", ""
+
 class CaptureManager:
     def __init__(self):
         self._method2capture = {}
@@ -118,15 +126,17 @@ class CaptureManager:
     def _makestringio(self):
         return py.io.TextIO() 
 
-    def _startcapture(self, method):
+    def _getcapture(self, method):
         if method == "fd": 
-            return py.io.StdCaptureFD(
+            return py.io.StdCaptureFD(now=False,
                 out=self._maketempfile(), err=self._maketempfile()
             )
         elif method == "sys":
-            return py.io.StdCapture(
+            return py.io.StdCapture(now=False,
                 out=self._makestringio(), err=self._makestringio()
             )
+        elif method == "no":
+            return NoCapture()
         else:
             raise ValueError("unknown capturing method: %r" % method)
 
@@ -152,27 +162,25 @@ class CaptureManager:
         if hasattr(self, '_capturing'):
             raise ValueError("cannot resume, already capturing with %r" % 
                 (self._capturing,))
-        if method != "no":
+        cap = self._method2capture.get(method)
-            cap = self._method2capture.get(method)
-            if cap is None:
-                cap = self._startcapture(method)
-                self._method2capture[method] = cap 
-            else:
-                cap.resume()
         self._capturing = method 
+        if cap is None:
+            self._method2capture[method] = cap = self._getcapture(method)
+            cap.startall()
+        else:
+            cap.resume()
 
     def suspendcapture(self, item=None):
         self.deactivate_funcargs()
         if hasattr(self, '_capturing'):
             method = self._capturing
-            if method != "no":
+            cap = self._method2capture.get(method)
-                cap = self._method2capture[method]
+            if cap is not None:
                 outerr = cap.suspend()
-            else:
-                outerr = "", ""
             del self._capturing
             if item:
-                outerr = (item.outerr[0] + outerr[0], item.outerr[1] + outerr[1])
+                outerr = (item.outerr[0] + outerr[0], 
+                          item.outerr[1] + outerr[1])
             return outerr 
         return "", ""
 
@@ -180,19 +188,17 @@ class CaptureManager:
         if not hasattr(pyfuncitem, 'funcargs'):
             return
         assert not hasattr(self, '_capturing_funcargs')
-        l = []
+        self._capturing_funcargs = capturing_funcargs = []
-        for name, obj in pyfuncitem.funcargs.items():
+        for name, capfuncarg in pyfuncitem.funcargs.items():
-            if name == 'capfd' and not hasattr(os, 'dup'):
-                py.test.skip("capfd funcarg needs os.dup")
             if name in ('capsys', 'capfd'):
-                obj._start()
+                capturing_funcargs.append(capfuncarg)
-                l.append(obj)
+                capfuncarg._start()
-        if l:
-            self._capturing_funcargs = l
 
     def deactivate_funcargs(self):
-        if hasattr(self, '_capturing_funcargs'):
+        capturing_funcargs = getattr(self, '_capturing_funcargs', None)
-            for capfuncarg in self._capturing_funcargs:
+        if capturing_funcargs is not None:
+            while capturing_funcargs:
+                capfuncarg = capturing_funcargs.pop()
                 capfuncarg._finalize()
             del self._capturing_funcargs
 
@@ -256,16 +262,19 @@ def pytest_funcarg__capfd(request):
     platform does not have ``os.dup`` (e.g. Jython) tests using
     this funcarg will automatically skip. 
     """ 
+    if not hasattr(os, 'dup'):
+        py.test.skip("capfd funcarg needs os.dup")
     return CaptureFuncarg(request, py.io.StdCaptureFD)
 
 
 class CaptureFuncarg:
     def __init__(self, request, captureclass):
         self._cclass = captureclass
+        self.capture = self._cclass(now=False)
         #request.addfinalizer(self._finalize)
 
     def _start(self):
-        self.capture = self._cclass()
+        self.capture.startall()
 
     def _finalize(self):
         if hasattr(self, 'capture'):
@@ -276,6 +285,4 @@ class CaptureFuncarg:
         return self.capture.readouterr()
 
     def close(self):
-        self.capture.reset()
+        self._finalize()
-        del self.capture
-

testing/io_/test_capture.py

@@ -96,6 +96,21 @@ def test_dupfile(tmpfile):
 class TestFDCapture: 
     pytestmark = needsdup 
 
+    def test_not_now(self, tmpfile):
+        fd = tmpfile.fileno()
+        cap = py.io.FDCapture(fd, now=False)
+        data = tobytes("hello")
+        os.write(fd, data)
+        f = cap.done()
+        s = f.read()
+        assert not s 
+        cap = py.io.FDCapture(fd, now=False)
+        cap.start()
+        os.write(fd, data)
+        f = cap.done()
+        s = f.read()
+        assert s == "hello"
+
     def test_stdout(self, tmpfile):
         fd = tmpfile.fileno()
         cap = py.io.FDCapture(fd)
@@ -185,8 +200,11 @@ class TestStdCapture:
     def test_capturing_twice_error(self):
         cap = self.getcapture() 
         print ("hello")
-        cap.reset()
+        out, err = cap.reset()
-        py.test.raises(Exception, "cap.reset()")
+        print ("world")
+        out2, err = cap.reset()
+        assert out == "hello\n"
+        assert not err
 
     def test_capturing_modify_sysouterr_in_between(self):
         oldout = sys.stdout 
@@ -210,7 +228,6 @@ class TestStdCapture:
         cap2 = self.getcapture() 
         print ("cap2")
         out2, err2 = cap2.reset()
-        py.test.raises(Exception, "cap2.reset()")
         out1, err1 = cap1.reset() 
         assert out1 == "cap1\n"
         assert out2 == "cap2\n"
@@ -265,6 +282,13 @@ class TestStdCapture:
         assert out == "after\n"
         assert not err 
 
+class TestStdCaptureNotNow(TestStdCapture):
+    def getcapture(self, **kw):
+        kw['now'] = False
+        cap = py.io.StdCapture(**kw)
+        cap.startall()
+        return cap
+
 class TestStdCaptureFD(TestStdCapture): 
     pytestmark = needsdup
 
@@ -296,6 +320,22 @@ class TestStdCaptureFD(TestStdCapture):
         assert out.startswith("3") 
         assert err.startswith("4") 
 
+class TestStdCaptureFDNotNow(TestStdCaptureFD):
+    pytestmark = needsdup
+
+    def getcapture(self, **kw): 
+        kw['now'] = False
+        cap = py.io.StdCaptureFD(**kw)
+        cap.startall()
+        return cap
+
+def test_capture_not_started_but_reset(): 
+    capsys = py.io.StdCapture(now=False)
+    capsys.done()
+    capsys.done()
+    capsys.reset()
+    capsys.reset()
+
 @needsdup
 def test_capture_no_sys(): 
     capsys = py.io.StdCapture()

testing/plugin/test_pytest_capture.py

@@ -39,6 +39,10 @@ class TestCaptureManager:
         old = sys.stdout, sys.stderr, sys.stdin
         try:
             capman = CaptureManager()
+            # call suspend without resume or start
+            outerr = capman.suspendcapture()
+            outerr = capman.suspendcapture()
+            assert outerr == ("", "")
             capman.resumecapture(method)
             print ("hello")
             out, err = capman.suspendcapture()