python / asyncio

asyncio historical repository
https://docs.python.org/3/library/asyncio.html
1.04k stars 177 forks source link

Don't select for read on character devices in _UnixWritePipeTransport #374

Closed ronf closed 8 years ago

ronf commented 8 years ago

Here's a pull request designed to fix the issue reported in #369.

_UnixWritePipeTransport selects for read on write pipes to detect when the remote end of the pipe is closed. This works for unidirectional FIFOs and dedicated socket pairs where nothing is written to the read side of the pair, but it can cause problems with devices or sockets where bidirectional I/O is being done.

This commit changes _UnixWritePipeTransport to only select for read on sockets and FIFOs (on OSes which support that), and not on character devices. When connect_write_pipe() is used on those devices, end-of-file will need to be detected using a read pipe which accepts inputs from the device.

No change is made to how sockets are handled, so passing in a socket used for bidirectional I/O to connect_write_pipe() is not supported. Async I/O on sockets should be performed using functions like BaseEventLoop.create_connection(). Socket pairs which are only used for undirectional I/O can be used here, though.

1st1 commented 8 years ago

I think we have a couple of tests for connect_write_pipe. Can we add more to test the edge case this PR solves?

ronf commented 8 years ago

The main thing that this change addresses is when the file object passed into connect_write_pipe() is a TTY object with both read & write happening on it. It looks like there are read & write pty tests already in test_events.py, but they don't specifically address the bidirectional I/O case. It should be possible to add another test for this, though. I'll see what I can put together.

If I get something working, should I just add it to this same pull request?

ronf commented 8 years ago

I've gone ahead and checked in a new unit test which exercises this change. Without the fix here, the unit test would fail, reporting that the write pipe had closed prematurely. With the fix, bidirectional I/O can be performed on PTY/TTY pair without an issue.

ronf commented 8 years ago

I don't think the failed test here has anything to do with this commit. It's testing socket code, and from looking at the test my guess is that it's a race condition related to random port assignment on the test system. It assumes that the random port that was selected won't be in use when it connects after closing the listener, but something else could have come along and re-opened that port in the interim.

ronf commented 8 years ago

Is there anything more I can do on this? I signed the Python Contributor Agreement tonight.

asvetlov commented 8 years ago

LGTM. I'll merge it tomorrow if nobody object

asvetlov commented 8 years ago

Thanks, @ronf

ronf commented 8 years ago

My pleasure. Thanks for merging the fix!