Skip to content
This repository was archived by the owner on Nov 23, 2017. It is now read-only.

Fix ordering issues in UNIX read/write pipe transport constructors #370

Closed
wants to merge 6 commits into from

Conversation

ronf
Copy link

@ronf ronf commented Jul 4, 2016

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

This commit re-orders the initialization code in the _UnixReadPipeTransport
and _UnixWritePipeTransport constructors to make sure all members are
assigned a value, even in the case where ValueError is raised due to
an incompatible type of pipe. This avoids exceptions being raised in
repr() due to the missing members.

In the case where ValueError is raised, this commit also clears the values
of _pipe, _fileno, and _protocol since this transport isn't returned,
avoiding a spurious "unclosed transport" warning when the object is
garbage-collected.

This commit re-orders the initialization code in the _UnixReadPipeTransport
and _UnixWritePipeTransport constructors to make sure all members are
assigned a value, even in the case where ValueError is raised due to
an incompatible type of pipe. This avoids exceptions being raised in
__repr__() due to the missing members.

In the case where ValueError is raised, this commit also clears the values
of _pipe, _fileno, and _protocol since this transport isn't returned,
avoiding a spurious "unclosed transport" warning when the object is
garbage-collected.
@ronf
Copy link
Author

ronf commented Jul 5, 2016

Sorry - I accidentally checked in a proposed fix to issue #369 into this same pull request. I should have used a separate branch for that. Let me know if you'd like me to split them.

@1st1
Copy link
Member

1st1 commented Jul 5, 2016

Hi Ron,

Sorry - I accidentally checked in a proposed fix to issue #369 into this same pull request. I should have used a separate branch for that. Let me know if you'd like me to split them.

Yes, please split them. That will make the review easier.

@gvanrossum
Copy link
Member

gvanrossum commented Jul 5, 2016 via email

@ronf
Copy link
Author

ronf commented Jul 6, 2016

Thanks - done. I'll submit the other patch in a separate pull request.

@1st1
Copy link
Member

1st1 commented Jul 6, 2016

Should we add a unittest checking that the repr works after a ValueError?

@ronf
Copy link
Author

ronf commented Jul 7, 2016

This is a bit tricky to do as a black-box test, as the object is not returned to the caller when a ValueError is raised, so there's nothing to call repr on from within the unit test code.

Jim Fulton and others added 5 commits August 30, 2016 20:55
…#387)

Signed-off-by: Sorin Sbarnea <sorin.sbarnea@gmail.com>
…python#374)

* Don't select for read on character devices in _UnixWritePipeTransport

_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.

* Add new unit test for bidirectional I/O on TTYs

* Don't import tty module on Windows

While the Windows release of Python seems to include the 'tty' module,
attempting to import it fails with an error about not finding the
'termios' module. Since the tty tests here aren't run on Windows,
though, there's no need to import tty there. This commit makes the
import conditional on the platform.
@ronf
Copy link
Author

ronf commented Aug 31, 2016

I tried to rebase this to fix a conflict with the merge of #374, but it didn't quite come out right. I'm going to close this and submit a new pull request with an updated fix for #368.

@ronf ronf closed this Aug 31, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants