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

Make trio.socket._SocketType.connect *always* close the socket on cancellation #247

Closed
njsmith opened this issue Jul 25, 2017 · 1 comment

Comments

@njsmith
Copy link
Member

njsmith commented Jul 25, 2017

Right now we have some awkward language saying that sometimes we do, sometimes we don't. It's not very helpful, and there's nothing particularly important about preserving an unconnected socket object -- if users really want it they can just create a new one. So we should just close it consistently.

@njsmith
Copy link
Member Author

njsmith commented Jun 16, 2018

There was some confusion in chat about whether this had already been done. Which makes sense, because look, here's some code in connect that closes the socket on cancellation:

trio/trio/_socket.py

Lines 706 to 713 in 65729b1

try:
await _core.wait_socket_writable(self._sock)
except _core.Cancelled:
# We can't really cancel a connect, and the socket is in an
# indeterminate state. Better to close it so we don't get
# confused.
self._sock.close()
raise

However, this only handles a cancellation that occurs during that call to wait_socket_writable. There are other places in the method that a cancellation could happen. So right now, if a cancellation happens in wait_socket_writable, then the socket is closed, but if it happens somewhere else inside connect, the socket isn't closed. That's kind of confusing. The idea in this issue is to make it so that any cancellation inside connect should leave the socket closed. Basically this just means taking the try/except I pasted above, and expanding it so that it wraps around the whole method, not just the call to wait_socket_writable.

(Thanks to @WindSoilder for pointing out the lack of clarity here.)

@sorcio sorcio closed this as completed in fd0d877 Jul 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant