Closed jayvdb closed 5 years ago
Hm. Is this a feature request, or a bug report (or both)?
I don't actually know all that much about properly working with streams -- can you explain the problem in more detail?
This prevents this package being usable if the code being tested needs to access the stdin.buffer , or any of the other features of the real stdin (and stdout/stderr) which are not currently implemented by TeeStdin.
Another important feature of the real stdin is the new reconfigure
in py37+.
But the current code isnt buggy, so you could also call it a feature when built ;-)
Aha, I failed to implement the full API of sys.stdin
, essentially? Definitely something to fix.
Did I miss anything on stdout
or stderr
?
All three are TextIOWrapper
, whereas atm you are using StringIO
for all three, so the same applies to them all.
Example of tests that need this
Examples of code that would need this
Worth noting however that TextIOWrapper is not thread safe. https://docs.python.org/3/library/io.html#multi-threading
If that is an issue, it shouldnt be hard to create a subclass/look-alike which is thread-safe because this mock wont have the same underlying problems that cause the threading problems, and the buffered readers/writers are threadsafe.
Nice, thank you for all the research & work on this and #25!
I definitely plan to accept the PR eventually, once I've had a chance to gain an understanding of what's going on. Are there any more aspects of this that should specifically be put under test in this repo, in addition to the detach
test you added?
Not more aspects of stdin, that I am aware of. The aspects that need to be considered are anything which could trip up the .close()
, as that the only extra thing that this code does to the stream.
I believe there is a leak potential in those closes, because if one of the first two throws an exception, and it is caught and handled properly, the others wont be closed if they were open. But that is a lot more complexity for that section of code, for a very obscure case, and some of the other issues provide solutions for that, or larger rewrites of that section of code.
Referring to an earlier comment, if sys.stdin
itself isn't thread-safe (shouldn't be, as a TextIOWrapper
subclass?), then I'm not concerned at present that stdio-mgr
be thread-safe, either. Probably would be a good thing to add explicitly to the docs, though. Maybe a note both in the TeeStdIn
docstring, and also somewhere suitable in README.rst
.
https://github.com/openstack/os-testr/blob/master/os_testr/tests/test_subunit_trace.py - this requires the context manager be given a binary stream, otherwise the read fails, and solving that without stdio_mgr accepting a binary stream makes the resulting code almost as complicated as the existing code.
https://github.com/jayvdb/os-testr/commit/eb34c02232e4023ed69d44c5fd2f7284e8a5ccaa
Captured traceback:
b'Traceback (most recent call last):' b' File "/home/jayvdb/projects/openstack/os-testr/os_testr/tests/test_subunit_trace.py", line 92, in test_trace' b' text = stream.read()' b' File "/home/jayvdb/projects/openstack/os-testr/.tox/py37/lib64/python3.7/codecs.py", line 322, in decode' b' (result, consumed) = self._buffer_decode(data, self.errors, final)' b"UnicodeDecodeError: 'utf-8' codec can't decode byte 0xb3 in position 0: invalid start byte"
Their tests pass with this https://github.com/jayvdb/stdio-mgr/commit/538bf530641c2fc2184e18ec723700ceba135769 and https://github.com/jayvdb/os-testr/commit/e9a6c85005b25a45ad061f6f9c3d940ccc57d7bc
sys.stdin
type_io.TextIOWrapper
has a.buffer
of type_io.BufferedReader
.