Codecov Report

Merging #1129 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1129      +/-   ##
==========================================
+ Coverage   99.55%   99.55%   +<.01%     
==========================================
  Files         105      105              
  Lines       12715    12716       +1     
  Branches      969      970       +1     
==========================================
+ Hits        12658    12659       +1     
  Misses         36       36              
  Partials       21       21

I'm trying to think of the reasons why we use open_* functions:

• When we can't use __init__ because we need a different calling convention (e.g. async open_file, or async context manager open_nursery)
• When we want to hide the actual type (open_nursery, the old open_cancel_scope) – but we're moving away from this (Better expose Nursery and TaskStatus for type annotations #778)
• When it's a convenience function that does some non-trivial setup (open_tcp_stream)

I don't think any of these apply here though? So we should probably expose the type(s) themselves instead of having open_* functions.

I also think it's awkward to have separate types for send and receive here... a generic fd supports both read and write, so it's weird to have to use StapledStream + dup to read-and-write to the same fd. Plus using dup here is a bit tricky, because we end up with two different objects trying to fiddle with the fcntl flags on the same underlying file description, so it increases the risk of them stomping on each other. And we don't have SocketSendStream and SocketReceiveStream either.

So I'm thinking maybe we should merge these together into a single trio.hazmat.FdStream.

This would mean that on Unix Process.stdin and friends are all FdStreams, even though they're unidirectional. But I think that's OK – if someone does accidentally try to call stdin.receive_some or stdout.send_all then at runtime that will raise an exception, and we'll statically declare those attributes to be a SendStream and ReceiveStream respectively, so mypy will also flag the error.

and we'll statically declare those attributes to be a SendStream and ReceiveStream respectively, so mypy will also flag the error.

I got lost here. If Process.stdin is typed as a FdStream, how can it also be declared as a SendStream?

FdStream is a subtype of SendStream, so we can statically type Process.stdin as SendStream, and then make it a FdStream at runtime. (And we certainly can't statically type it as FdStream, because it will be something else on Windows...)

I've implemented FdStream. Not quite sure how Process.stdio should be declared in the Unix case.

Aside: I believe StapledStream send_stream and receive_stream attributes should be hidden. Anyway changing Process.stdio to FdStream instance in the Unix case is going to break access to them.

Unfortunately the StapledStream API documentation is referring to those attributes extensively... Do inherited method docs really need to be repeated in derived class?

oh, but this:

    But if you read the docs for :class:`~trio.StapledStream` and
    :func:`memory_stream_one_way_pair`, you'll see that all the pieces
    involved in wiring this up are public APIs, so you can adjust to suit the
    requirements of your tests. For example, here's how to tweak a stream so
    that data flowing from left to right trickles in one byte at a time (but
    data flowing from right to left proceeds at full speed)::

        left, right = memory_stream_pair()
        async def trickle():
            # left is a StapledStream, and left.send_stream is a MemorySendStream
            # right is a StapledStream, and right.recv_stream is a MemoryReceiveStream
            while memory_stream_pump(left.send_stream, right.recv_stream, max_byes=1):
                # Pause between each byte
                await trio.sleep(1)

I would expect that we'd have:

Process.stdin: typed as SendStream, on Unix actually an FdStream
Process.stdout, Process.stderr: typed as ReceiveStream, on Unix actually an FdStream
Process.stdio: StapledStream(stdin, stdout)

Note that on Unix we use pipes to talk to the child process, and pipes are unidirectional, so stdio has to be a StapledStream...

Oh I see, you're defining FdStream as managing two fds, so it kind of has the StapledStream functionality baked into it. I was suggesting making FdStream manage a single fd, similar to SocketStream.

Oh I see, you're defining FdStream as managing two fds

It did make it easy to ensure only one holder when receive and send streams were the same. Any ideas for that given the single-fd FdStream?

If the send and receive streams are the same, then just... Use one FdStream? I don't think I'm understanding the question.

OK-- I think the API is settled. Added tests (from former functions) and docs.

I didn't do anything to preemptively check for accidental send_all / receive_some on the wrong mode at runtime. I'm assuming it's fine to defer to the existing OSError handling.

another thing I wasn't quite sure about: in the implementation I've used separate task conflict detectors for send & receive

NOTE: please squash on merge

the only reason _FdHolder exists is to factor out some common code from PipeSendStream and PipeReceiveStream. Now that we're merging those two classes into one, it's no longer really serving any purpose, and we could merge it back into FdStream.

I'm inclined to keep the separation as it keeps FdStream readable. FdHolder could have its own test.

I made a few trivial wording tweaks, and otherwise looks great! Feel free to merge once CI passes, in case I forget...

Actually, I take that back :-). The coverage report is saying that from ._unix_pipes import FdStream always succeeds, even on Windows: https://codecov.io/gh/python-trio/trio/pull/1129/src/trio/hazmat.py?before=trio/hazmat.py

Which makes sense, because there's nothing in trio._unix_pipes that depends on platform-specific stuff at import time. There used to be, when we were importing fcntl, but not anymore. So I guess right now we're exporting trio.hazmat.FdStream on Windows. But of course if anyone tries to use it on Windows then it will just crash.

(Side note: it's too bad the tests don't catch this... I guess they're doing skipif(not os.name == "posix"), not skipif(not hasattr(trio.hazmat, "FdStream")?)

The obvious solution would be to make it if os.name == "posix": from ._unix_pipes import FdStream, but from your commit comments, I guess jedi didn't like that. Which is irritating :-/. Annoyingly, mypy doesn't understand it either: https://mypy.readthedocs.io/en/latest/common_issues.html#version-and-platform-checks

One option would be to use if os.name == "posix", and accept that jedi won't autocomplete correctly, and hack the jedi test to add a special-case exception

Another option, which is kind of a silly hack, but I guess would work: at the top of _unix_pipes.py, do from os import get_blocking, set_blocking, which will raise ImportError on Windows. Of course this would need a comment :-)

I'm now manually raising ImportError from _unix_pipes.py, and added coverage for this.

Looks great, waiting for CI now...