Fix _UnixWritePipeTransport to support TTY

Fix _UnixWritePipeTransport to support TTY

[GvR, 2014-02-19 23:12:21]

https://codereview.appspot.com/66190043/diff/1/asyncio/unix_events.py
File asyncio/unix_events.py (right):

https://codereview.appspot.com/66190043/diff/1/asyncio/unix_events.py#newcode346
asyncio/unix_events.py:346: and not pipe.isatty()):
I think this ought to be grouped differently, like so:

if <SOCKET> or (not <AIX> and not <TTY>):

[haypo_gmail, 2014-02-20 08:14:46]

https://codereview.appspot.com/66190043/diff/1/asyncio/unix_events.py
File asyncio/unix_events.py (right):

https://codereview.appspot.com/66190043/diff/1/asyncio/unix_events.py#newcode346
asyncio/unix_events.py:346: and not pipe.isatty()):
On 2014/02/19 23:12:21, GvR wrote:
> I think this ought to be grouped differently, like so:
> 
> if <SOCKET> or (not <AIX> and not <TTY>):

Ah yes correct, fixed in new patch.

[GvR, 2014-02-20 08:35:46]

LGMT.


On Thu, Feb 20, 2014 at 12:14 AM, <victor.stinner@gmail.com> wrote:

> Reviewers: GvR,
>
>
>
> https://codereview.appspot.com/66190043/diff/1/asyncio/unix_events.py
> File asyncio/unix_events.py (right):
>
> https://codereview.appspot.com/66190043/diff/1/asyncio/
> unix_events.py#newcode346
> asyncio/unix_events.py:346: and not pipe.isatty()):
> On 2014/02/19 23:12:21, GvR wrote:
>
>> I think this ought to be grouped differently, like so:
>>
>
>  if <SOCKET> or (not <AIX> and not <TTY>):
>>
>
> Ah yes correct, fixed in new patch.
>
> Description:
> Fix _UnixWritePipeTransport to support TTY
>
> Please review this at https://codereview.appspot.com/66190043/
>
> Affected files (+32, -1 lines):
>   M asyncio/unix_events.py
>   A examples/tty.py
>
>
> Index: asyncio/unix_events.py
> ===================================================================
> --- a/asyncio/unix_events.py
> +++ b/asyncio/unix_events.py
> @@ -342,7 +342,8 @@ class _UnixWritePipeTransport(transports
>          # On AIX, the reader trick only works for sockets.
>          # On other platforms it works for pipes and sockets.
>          # (Exception: OS X 10.4?  Issue #19294.)
> -        if is_socket or not sys.platform.startswith("aix"):
> +        if (is_socket
> +            or (not sys.platform.startswith("aix") and not
> pipe.isatty())):
>              self._loop.add_reader(self._fileno, self._read_ready)
>
>          self._loop.call_soon(self._protocol.connection_made, self)
> Index: examples/tty.py
> ===================================================================
> new file mode 100644
> --- /dev/null
> +++ b/examples/tty.py
> @@ -0,0 +1,30 @@
> +"""
> +Copy stdin into stdout, line by line.
> +"""
> +
> +import asyncio
> +import sys
> +from asyncio import streams
> +
> +class StdoutProtocol(streams.FlowControlMixin,
> +                     asyncio.Protocol):
> +    pass
> +
> +@asyncio.coroutine
> +def copy_stdin_to_stdout():
> +    reader = asyncio.StreamReader()
> +    reader_protocol = asyncio.StreamReaderProtocol(reader)
> +    yield from loop.connect_read_pipe(lambda: reader_protocol,
> +                                      sys.stdin)
> +    transport, protocol = yield from loop.connect_write_pipe(
> StdoutProtocol,
> +                                                             sys.stdout)
> +    writer = streams.StreamWriter(transport, protocol, reader, loop)
> +    while True:
> +        line = yield from reader.readline()
> +        if not line:
> +            break
> +        writer.write(line)
> +        yield from writer.drain()
> +
> +loop = asyncio.get_event_loop()
> +loop.run_until_complete(copy_stdin_to_stdout())
>
>
>


-- 
--Guido van Rossum (python.org/~guido)

[haypo_gmail, 2014-02-20 10:05:37]

On 2014/02/20 08:35:46, GvR wrote:
> LGMT.

Hum, no, it's not good yet. The cat program exits after the first line because
the TTY is set to non-blocking mode:
---
$ cat|PYTHONPATH=$PWD ~/prog/python/3.3/python examples/tty.py  
abcdef
cat: -: Resource temporarily unavailable
abcdef
---

The new patch doesn't set non-blocking mode if the pipe is a TTY.

[GvR, 2014-02-20 16:01:05]

I do not understand why you don't make the FD nonblocking when it's a pipe.
Won't that cause blocking for I/O if you write too much data to it too fast?

Another worry: not all file-like objects may implement .isatty().

All in all worry that this use case is pretty marginal and not worth breaking
some other edge case in 3.4.

Please do not submit.

[haypo_gmail, 2014-02-20 16:16:10]

On 2014/02/20 16:01:05, GvR wrote:
> I do not understand why you don't make the FD nonblocking when it's a pipe.

(Not for any pipe, only if it's a TTY.) Because it breaks the cat program: "cat:
-: Resource temporarily unavailable" error in my example.

> Won't that cause blocking for I/O if you write too much data to it too fast?

I don't understand how the read and the write file descriptors are connected. In
"cat|PYTHONPATH=$PWD ~/prog/python/3.3/python examples/tty.py", I expect that
Python stdin is a pipe disconnected from python stdout (TTY). It looks like cat
stdin is the same TTY and that the non-blocking mode is shared between cat stdin
and python stdout.

> Another worry: not all file-like objects may implement .isatty().

I first wrote os.isatty(self._fileno). You're right, it's probably better.

> All in all worry that this use case is pretty marginal and not worth breaking
> some other edge case in 3.4.

Agreed.

[xdegaye, 2014-12-19 16:37:07]

When running the example with:
    python -c "while 1: print(7 * '0123456789')" | python examples/tty.py | cat

I get the following exception on linux (same result when the patch is applied or
not since stdin and stdout are connected to different pipes anyway):
protocol.resume_writing() failed
protocol: <__main__.StdoutProtocol object at 0x7ff2f6762538>
transport: <_UnixWritePipeTransport fd=1 idle bufsize=0>
Traceback (most recent call last):
  File "Lib/asyncio/transports.py", line 269, in _maybe_resume_protocol
    self._protocol.resume_writing()
  File "Lib/asyncio/streams.py", line 162, in resume_writing
    if self._loop.get_debug():
AttributeError: 'NoneType' object has no attribute 'get_debug'


This is fixed by replacing the call to connect_write_pipe() in tty.py with:
    transport, protocol = yield from loop.connect_write_pipe(lambda:
StdoutProtocol(loop), sys.stdout)

Not sure why FlowControlMixin does not check for self._loop attributes given the
comment in its constructor.

[haypo_gmail, 2014-12-19 17:59:35]

Hi xdegaye,

This old review is not the best place to discuss the TTY issue. You should
discuss it on the Tulip mailing list, Tulip bug tracker, or event the Python bug
tracker, to get more feedback.

I'm not sure that it's possible to support correctly TTY in Python, I mean not
using file descriptors.

Victor