Return the correct value from Popen.communicate() on Py3/text mode

With tests, verified against the stdlib.

Fixes a todo, and fixes #939.

CHANGES.rst

@@ -46,6 +46,11 @@
 - Linux CI now tests on PyPy3 3.5-5.8.0, updated from PyPy3 3.5-5.7.1.
   See :issue:`1001`.
 
+- :meth:`gevent.subprocess.Popen.communicate` returns the correct type
+  of str (not bytes) in universal newline mode under Python 3, or when
+  an encoding has been specified. Initial patch in :pr:`939` by
+  William Grzybowski.
+
 1.2.2 (2017-06-05)
 ==================
 

src/gevent/subprocess.py

@@ -387,6 +387,10 @@ class Popen(object):
        restricted to Python 3.
     """
 
+    # The value returned from communicate() when there was nothing to read.
+    # Changes if we're in text mode or universal newlines mode.
+    _communicate_empty_value = b''
+
     def __init__(self, args, bufsize=None, executable=None,
                  stdin=None, stdout=None, stderr=None,
                  preexec_fn=None, close_fds=_PLATFORM_DEFAULT_CLOSE_FDS, shell=False,
@@ -504,6 +508,11 @@ class Popen(object):
                 errread = msvcrt.open_osfhandle(errread.Detach(), 0)
 
         text_mode = PY3 and (self.encoding or self.errors or universal_newlines)
+        if text_mode or universal_newlines:
+            # Always a native str in universal_newlines mode, even when that
+            # str type is bytes. Additionally, text_mode is only true under
+            # Python 3, so it's actually a unicode str
+            self._communicate_empty_value = ''
 
         if p2cwrite is not None:
             if PY3 and text_mode:
@@ -643,31 +652,33 @@ class Popen(object):
         # that no output should be lost in the event of a timeout.) Instead, we're
         # watching for the exception and ignoring it. It's not elegant,
         # but it works
-        if self.stdout:
-            def _read_out():
+        def _make_pipe_reader(pipe_name):
+            pipe = getattr(self, pipe_name)
+            buf_name = '_' + pipe_name + '_buffer'
+
+            def _read():
                 try:
-                    data = self.stdout.read()
+                    data = pipe.read()
                 except RuntimeError:
                     return
-                if self._stdout_buffer is not None:
-                    self._stdout_buffer += data
+                if not data:
+                    return
+                the_buffer = getattr(self, buf_name)
+                if the_buffer:
+                    the_buffer.append(data)
                 else:
-                    self._stdout_buffer = data
+                    setattr(self, buf_name, [data])
+            return _read
+
+        if self.stdout:
+            _read_out = _make_pipe_reader('stdout')
             stdout = spawn(_read_out)
             greenlets.append(stdout)
         else:
             stdout = None
 
         if self.stderr:
-            def _read_err():
-                try:
-                    data = self.stderr.read()
-                except RuntimeError:
-                    return
-                if self._stderr_buffer is not None:
-                    self._stderr_buffer += data
-                else:
-                    self._stderr_buffer = data
+            _read_err = _make_pipe_reader('stderr')
             stderr = spawn(_read_err)
             greenlets.append(stderr)
         else:
@@ -686,25 +697,30 @@ class Popen(object):
         if timeout is not None and len(done) != len(greenlets):
             raise TimeoutExpired(self.args, timeout)
 
-        if self.stdout:
-            try:
-                self.stdout.close()
-            except RuntimeError:
-                pass
-        if self.stderr:
-            try:
-                self.stderr.close()
-            except RuntimeError:
-                pass
+        for pipe in (self.stdout, self.stderr):
+            if pipe:
+                try:
+                    pipe.close()
+                except RuntimeError:
+                    pass
+
         self.wait()
-        stdout_value = self._stdout_buffer
-        self._stdout_buffer = None
-        stderr_value = self._stderr_buffer
-        self._stderr_buffer = None
-        # XXX: Under python 3 in universal newlines mode we should be
-        # returning str, not bytes
-        return (None if stdout is None else stdout_value or b'',
-                None if stderr is None else stderr_value or b'')
+
+        def _get_output_value(pipe_name):
+            buf_name = '_' + pipe_name + '_buffer'
+            buf_value = getattr(self, buf_name)
+            setattr(self, buf_name, None)
+            if buf_value:
+                buf_value = self._communicate_empty_value.join(buf_value)
+            else:
+                buf_value = self._communicate_empty_value
+            return buf_value
+
+        stdout_value = _get_output_value('stdout')
+        stderr_value = _get_output_value('stderr')
+
+        return (None if stdout is None else stdout_value,
+                None if stderr is None else stderr_value)
 
     def poll(self):
         """Check if child process has terminated. Set and return :attr:`returncode` attribute."""

src/greentest/greentest.py

@@ -133,6 +133,8 @@ if PYPY3:
 else:
     skipOnPyPy3 = _do_not_skip
 
+skipIf = unittest.skipIf
+
 EXPECT_POOR_TIMER_RESOLUTION = PYPY3 or RUNNING_ON_APPVEYOR
 
 class ExpectedException(Exception):

src/greentest/test__subprocess.py

@@ -4,6 +4,10 @@ import errno
 import greentest
 import gevent
 from gevent import subprocess
+
+if not hasattr(subprocess, 'mswindows'):
+    # PyPy3, native python subprocess
+    subprocess.mswindows = False
 import time
 import gc
 import tempfile
@@ -230,6 +234,36 @@ class Test(greentest.TestCase):
 
         test_subprocess_in_native_thread.ignore_leakcheck = True
 
+    def __test_no_output(self, kwargs, kind):
+        proc = subprocess.Popen([sys.executable, '-c', 'pass'],
+                                stdout=subprocess.PIPE,
+                                **kwargs)
+        stdout, stderr = proc.communicate()
+
+        self.assertIsInstance(stdout, kind)
+        self.assertIsNone(stderr)
+
+
+    def test_universal_newlines_text_mode_no_output_is_always_str(self):
+        # If the file is in universal_newlines mode, we should always get a str when
+        # there is no output.
+        # https://github.com/gevent/gevent/pull/939
+        kwargs = {'universal_newlines': True}
+        self.__test_no_output({'universal_newlines': True}, str)
+
+    @greentest.skipIf(sys.version_info[:2] < (3, 6), "Need encoding argument")
+    def test_encoded_text_mode_no_output_is_str(self):
+        # If the file is in universal_newlines mode, we should always get a str when
+        # there is no output.
+        # https://github.com/gevent/gevent/pull/939
+        self.__test_no_output({'encoding': 'utf-8'}, str)
+
+    def test_default_mode_no_output_is_always_str(self):
+        # If the file is in default mode, we should always get a str when
+        # there is no output.
+        # https://github.com/gevent/gevent/pull/939
+        self.__test_no_output({}, bytes)
+
 class RunFuncTestCase(greentest.TestCase):
     # Based on code from python 3.6