issue 247, any cancelation inside the socket will leave it closed #574
Conversation
Codecov Report
@@ Coverage Diff @@
## master #574 +/- ##
==========================================
+ Coverage 99.27% 99.27% +<.01%
==========================================
Files 89 89
Lines 10617 10628 +11
Branches 747 747
==========================================
+ Hits 10540 10551 +11
Misses 59 59
Partials 18 18
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
👍 I think this is good and the test is testing the right thing. I should note that I accidentally misguided you (when we discussed this at EuroPython) by saying that the connect method can only be cancelled in two points, but there is a third one that I didn't notice. The async with _try_sync() line is one more point where cancellation can happen, on enter. Probably it's fine and we don't want to add a different test case, but I think I should let you know because I said something different before, and maybe you want to give a second look at the code.
When you feel you're done, can you view the steps in the contributing guide here? https://trio.readthedocs.io/en/latest/contributing.html#preparing-pull-requests
Specifically running yapf and adding a newsfragment :)
trio/tests/test_socket.py
Outdated
| @@ -10,6 +10,7 @@ | |||
| from .. import _socket as _tsocket | |||
| from .. import socket as tsocket | |||
| from .._socket import _NUMERIC_ONLY, _try_sync | |||
| from .._timeouts import sleep | |||
There was a problem hiding this comment.
Maybe we can avoid importing from _timeouts here if in the test below you call _core.checkpoint() rather than sleep.
trio/tests/test_socket.py
Outdated
| @@ -506,6 +507,7 @@ class MySocket(stdlib_socket.socket): | |||
| with assert_checkpoints(): | |||
| with pytest.raises(TypeError): | |||
| await ta.recv("haha") | |||
|
|
|||
There was a problem hiding this comment.
Careful, you're introducing spurious whitespace :)
trio/tests/test_socket.py
Outdated
| @@ -515,6 +517,7 @@ class MySocket(stdlib_socket.socket): | |||
| nursery.start_soon(do_successful_blocking_recv) | |||
| await wait_all_tasks_blocked() | |||
| b.send(b"2") | |||
|
|
|||
trio/tests/test_socket.py
Outdated
|
|
||
| async def _resolve_remote_address(self, *args, **kwargs): | ||
| cancel_scope.cancel() | ||
| await sleep(.001) |
There was a problem hiding this comment.
Here is where I would recommend to use _core.checkpoint().
|
@sorcio switched the sleep for the checkpoint and removed the extra lines you mentioned |
|
Thanks, and welcome! 🎉 🎂 And, no pressure, but if you'd like to keep contributing then we'd love to have you, so I'm sending you a github invite now. You can read more about this in our contributing documentation. |
No description provided.