Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

POC: Reconfigure existing streams #78

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Sep 6, 2019

Implements #75

Sits on top of #76

Failing on Windows, and other contexts.

This would be optional, or auto-used when the environment is acceptable.

@codecov-io
Copy link

codecov-io commented Sep 7, 2019

Codecov Report

Merging #78 into master will decrease coverage by 8.54%.
The diff coverage is 88.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #78      +/-   ##
==========================================
- Coverage     100%   91.45%   -8.55%     
==========================================
  Files           3        5       +2     
  Lines         101      281     +180     
==========================================
+ Hits          101      257     +156     
- Misses          0       24      +24
Impacted Files Coverage Δ
src/stdio_mgr/compat.py 100% <100%> (ø)
src/stdio_mgr/stdio_mgr.py 89.15% <83.7%> (-10.85%) ⬇️
src/stdio_mgr/types.py 98.38% <98.38%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee7efe2...3d9570f. Read the comment docs.

@jayvdb
Copy link
Contributor Author

jayvdb commented Sep 7, 2019

ae14f2e is a slightly different approach, which is failing occasionally, especially with repeated use, but I expect we can get this working on Windows also. fingers crossed there is a minor problem in my file handling.

Using PYTHONUNBUFFERED=1 is only to facilitate setting up the streams in a way to test this approach, and make the Unix CI more like the Windows CI; it would be used more generally whenever the sys stream buffers are a FileIO.

17174a3 would still be used, especially on non-Windows, but I didnt want to include lots of branching between two implementations.

Very likely the eventual solution will need to dynamically choose the best method of injecting new streams into the existing streams, and possibly even fall back to the current master approach of replacing sys.std* if the environment has sys handles which do not allow injection of new streams.

Given how brittle this is, we probably need PyPy and other Python implementations in the CI before it lands, but no need to get ahead on that, as this PYTHONUNBUFFERED=1 mode may not work at all, which would mean only 17174a3 works, and that doesnt need extra CI as it is only using exposed members.

@jayvdb jayvdb force-pushed the reconf branch 2 times, most recently from f3ec6f9 to 63ad755 Compare September 8, 2019 02:06
@jayvdb
Copy link
Contributor Author

jayvdb commented Sep 8, 2019

OK, so we definitely have something usable here, and #75 (comment) has yet another way to achieve the same which should be less brittle. ~~ There is some ugly logging happening in the repeated test case, but that just needs some cleaning up, ~~ (fixed) and the manual close tests appear to really close stdout which means the pytest output thereafter is lost or the process may even disappears, and there are a few other finicky details wrt fake-closing the real sys streams.

Note unbuffered is the default on Python 3.7, but worth noting that Git Bash for Windows apparently causes Python 3.7 to go into buffered mode. Point being that if there are ugly cludges, or remaining scenarios where it doesnt work properly, they will be older Pythons, and we'll degrade gracefully of course.

Definitely need PyPy and IronPython and Jython CI now so we can aim for high compatibility and document incompatibility where it isnt feasible (easily).

@jayvdb
Copy link
Contributor Author

jayvdb commented Sep 8, 2019

Next trick is to detect when this isnt possible, such as when pytest is used without -s, and fallback to the non-injecting current master implementation.

Creates compat.py with backport of contextlib.AbstractContextManager
to Python 3.4, so that the external interface of StdioManager
has a consistent MRO irrespective of the Python version.
StdioTupleBase combines MultiItemTuple and TupleContextManager,
and provides properties `stdin`, `stdout` and `stderr` for
literate referencing the three members of the tuple.

MultiItemTuple provides _map(), _all() and _any() for iterables,
with suppress versions able to ignore exceptions.

Base TupleContextManager creates a consistent MRO of tuple and
AbstractContextManager.

ClosingStdioTuple is a sample context manager which performs close()
on each item in the tuple.

TextIOTuple requires items to be a TextIOBase.

AnyIOTuple can be used to create either a TextIOTuple or a FakeIOTuple,
depending on whether the items are all a TextIOBase.  This will allow
attaching different implementations to each.

AnyIOTuple is used to capture `sys.__std*__` and `sys.std*` as they
existed at module import.
@bskinn
Copy link
Owner

bskinn commented Sep 9, 2019

General question -- how does bringing the "File" types of objects into things relate to the stream-reconfiguration machinery?

@bskinn
Copy link
Owner

bskinn commented Sep 9, 2019

Separately, please take a look at the two Projects I just created for the repo. It would really help my understanding if the higher-level concepts were curated in a way keeps all the related stuff together, regardless of how spread out they are around the various issues/PRs.

Many existing stdio capture and wrapper streams are only partial
implementations, most commonly missing `__enter__` and `__exit__`.
When StdioManager needs to integrate those streams into its service
it needs to suppress exceptions caused by those missing methods.
Also fix test_capture_stderr_warn: Use always filter
@jayvdb
Copy link
Contributor Author

jayvdb commented Sep 10, 2019

As expected, pypy3 is failing. It looks pretty easy to fix.

General question -- how does bringing the "File" types of objects into things relate to the stream-reconfiguration machinery?

Hopefully #75 (comment) provides enough context.

https://github.com/bskinn/stdio-mgr/projects

I've got a few things bunched up in this POC PR, which will feed back into new issues and new smaller PRs. I'll try to group them into projects.

@bskinn
Copy link
Owner

bskinn commented Sep 11, 2019

I may have found a way to diagnose python -u?

C:\> python -c "import sys; print(sys.__stdout__.buffer)"
<_io.BufferedWriter name='<stdout>'>

C:\> python -u -c "import sys; print(sys.__stdout__.buffer)"
<_io._WindowsConsoleIO mode='wb' closefd=False>

@jayvdb
Copy link
Contributor Author

jayvdb commented Sep 11, 2019

👍 classnames like _io._WindowsConsoleIO are a good indicator on Windows, but not so clear for linux.

> python3 -u -c "import sys; print(sys.__stdout__.buffer)"
<_io.FileIO name='<stdout>' mode='wb' closefd=False>

But, using sys.__stdout__ for this is perfect, I didnt think of that. I guess we can also use name='<stdout>' / closefd=False / and fileno() of 1 or 2 as more indicators that it is the real original and is unbuffered.

@bskinn
Copy link
Owner

bskinn commented Sep 11, 2019

On Debian:

$ python3 -c "import sys; print(sys.__stdout__.buffer)"
<_io.BufferedWriter name='<stdout>'>
$ python3 -u -c "import sys; print(sys.__stdout__.buffer)"
<_io.FileIO name='<stdout>' mode='wb' closefd=False>

IOW, it seems like if it's buffered (no -u), it's an io.BufferedWriter...whereas if it's with -u or PYTHONBUFFERED=1, it's some more raw type.

$ env PYTHONUNBUFFERED=1 python3 -c "import sys; print(sys.__stdout__.buffer)"
<_io.FileIO name='<stdout>' mode='wb' closefd=False>

@bskinn
Copy link
Owner

bskinn commented Sep 11, 2019

The detection is even more fundamental than that:

>>> bf = io.BufferedWriter(io.BytesIO())
>>> bf.closefd
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: '_io.BufferedWriter' object has no attribute 'closefd'
>>> bf.fileno()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
io.UnsupportedOperation: fileno

@bskinn bskinn mentioned this pull request Sep 16, 2019
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants