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

bpo-31922: Do not connect UDP sockets when broadcast is allowed #423

Merged
merged 1 commit into from
May 7, 2019

Conversation

vxgmichel
Copy link
Contributor

@vxgmichel vxgmichel commented Mar 3, 2017

Moved from python/asyncio#493.

This PR fixes issue python/asyncio#480, as explained in this comment.

The _SelectorDatagramTransport.sendto method has to be modified so _sock.sendto is used in all cases (because it is tricky to reliably tell if the socket is connected or not). Could that be an issue for connected sockets? EDIT ... so _sock.send is used only if _sock is connected.

It also protects socket.getsockname against OSError in _SelectorTransport. This might happen on Windows if the socket is not connected (e.g. for UDP broadcasting).

https://bugs.python.org/issue31922

@mention-bot
Copy link

@vxgmichel, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Haypo, @1st1, @asvetlov, @vadmium and @tiran to be potential reviewers.

@1st1 1st1 requested a review from asvetlov October 19, 2017 17:34
@vxgmichel vxgmichel requested a review from 1st1 as a code owner November 2, 2017 11:11
@vxgmichel vxgmichel changed the title asyncio: Do not connect UDP sockets when broadcast is allowed bpo-31922: Do not connect UDP sockets when broadcast is allowed Nov 2, 2017
@1st1
Copy link
Member

1st1 commented May 29, 2018

Vincent, could you please rebase your PR? If @asvetlov approves this can go into 3.7.1.

@vxgmichel vxgmichel force-pushed the asyncio-issue-480 branch 2 times, most recently from 9e956af to c1557f4 Compare May 29, 2018 23:39
Lib/asyncio/selector_events.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@vxgmichel
Copy link
Contributor Author

vxgmichel commented May 30, 2018

@asvetlov @1st1 The macOS CI fails with the following error:

======================================================================
ERROR: test_create_datagram_endpoint (test.test_asyncio.test_events.SelectEventLoopTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/vsts/agent/2.133.3/work/1/s/Lib/test/test_asyncio/test_events.py", line 1367, in test_create_datagram_endpoint
    test_utils.run_until(self.loop, lambda: server.nbytes)
  File "/Users/vsts/agent/2.133.3/work/1/s/Lib/test/test_asyncio/utils.py", line 114, in run_until
    raise futures.TimeoutError()
concurrent.futures._base.TimeoutError
----------------------------------------------------------------------

I suspect it is related to the following change:

@@ -992,10 +997,7 @@ class _SelectorDatagramTransport(_SelectorTransport):
         if not self._buffer:
             # Attempt to send it right away first.
             try:
-                if self._address:
-                    self._sock.send(data)
-                else:
-                    self._sock.sendto(data, addr)
+                self._sock.sendto(data, addr)

However, I have no idea why send would work differently from sendto in the context of this test on macOS specifically. Since I don't have access to a machine with this operating system, I can't investigate the problem on my own. Please let me know if you need more information about those changes.

@vxgmichel
Copy link
Contributor Author

After some investigation I realized that using sendto on a connected socket raises the following OSError on macOS:

[Errno 56] Socket is already connected

Similar issues:

I'll update the PR.

@vxgmichel vxgmichel force-pushed the asyncio-issue-480 branch 4 times, most recently from a303db1 to 459aced Compare June 7, 2018 23:22
@vstinner
Copy link
Member

vstinner commented Jun 8, 2018

AppVeyor failed with:

======================================================================
FAIL: test_unawaited_warning_during_shutdown (test.test_coroutines.UnawaitedWarningDuringShutdownTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\cpython\lib\test\test_coroutines.py", line 2369, in test_unawaited_warning_during_shutdown
    assert_python_ok("-c", code)
  File "C:\projects\cpython\lib\test\support\script_helper.py", line 157, in assert_python_ok
    return _assert_python(True, *args, **env_vars)
  File "C:\projects\cpython\lib\test\support\script_helper.py", line 143, in _assert_python
    res.fail(cmd_line)
  File "C:\projects\cpython\lib\test\support\script_helper.py", line 84, in fail
    err))
AssertionError: Process return code is 3221225477
command line: ['C:\\projects\\cpython\\PCbuild\\win32\\python.exe', '-X', 'faulthandler', '-I', '-c', 'import asyncio\nasync def f(): pass\nasyncio.gather(f())\n']
stdout:
---
---
stderr:
---
Windows fatal exception: access violation
Current thread 0x00000d30 (most recent call first):
  File "C:\projects\cpython\lib\asyncio\events.py", line 41 in __init__
  File "C:\projects\cpython\lib\asyncio\selector_events.py", line 260 in _add_reader
  File "C:\projects\cpython\lib\asyncio\selector_events.py", line 117 in _make_self_pipe
  File "C:\projects\cpython\lib\asyncio\selector_events.py", line 66 in __init__
  File "C:\projects\cpython\lib\asyncio\events.py", line 660 in new_event_loop
  File "C:\projects\cpython\lib\asyncio\events.py", line 640 in get_event_loop
  File "C:\projects\cpython\lib\asyncio\tasks.py", line 576 in ensure_future
  File "C:\projects\cpython\lib\asyncio\tasks.py", line 715 in gather
  File "<string>", line 3 in <module>
---

I have a good news for you: it's your lucky day! @1st1 and me already identified this bug and it's already fixed in 3.7 and master branches! Please rebase your change on top of master.

FYI the bug was: https://bugs.python.org/issue33803

@vxgmichel
Copy link
Contributor Author

vxgmichel commented Jun 8, 2018

Hi @vstinner, the CI is fixed, thanks!

This fixes asyncio issue python#480.
The _SelectorDatagramTransport.sendto method has to be modified
so _sock.send is used only if _sock is connected.

It also protects socket.getsockname against OSError in
_SelectorTransport. This might happen on Windows if the socket
is not connected (e.g. for UDP broadcasting).
@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

1 similar comment
@miss-islington
Copy link
Contributor

Sorry, I can't merge this PR. Reason: Base branch was modified. Review and try the merge again..

@miss-islington
Copy link
Contributor

Thanks @vxgmichel for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 7, 2019
…onGH-423)

*Moved from python/asyncioGH-493.*

This PR fixes issue python/asyncioGH-480, as explained in [this comment](https://github.com/python/asyncio/issues/480GH-issuecomment-278703828).

The `_SelectorDatagramTransport.sendto` method has to be modified ~~so `_sock.sendto` is used in all cases (because it is tricky to reliably tell if the socket is connected or not). Could that be an issue for connected sockets?~~ *EDIT* ... so `_sock.send` is used only if `_sock` is connected.

It also protects `socket.getsockname` against `OSError` in `_SelectorTransport`. This might happen on Windows if the socket is not connected (e.g. for UDP broadcasting).

https://bugs.python.org/issue31922
(cherry picked from commit 63deaa5)

Co-authored-by: Vincent Michel <[email protected]>
@bedevere-bot
Copy link

GH-13162 is a backport of this pull request to the 3.7 branch.

@asvetlov
Copy link
Contributor

asvetlov commented May 7, 2019

Thanks and sorry for long waiting.

miss-islington added a commit that referenced this pull request May 7, 2019
*Moved from python/asyncioGH-493.*

This PR fixes issue python/asyncioGH-480, as explained in [this comment](https://github.com/python/asyncio/issues/480GH-issuecomment-278703828).

The `_SelectorDatagramTransport.sendto` method has to be modified ~~so `_sock.sendto` is used in all cases (because it is tricky to reliably tell if the socket is connected or not). Could that be an issue for connected sockets?~~ *EDIT* ... so `_sock.send` is used only if `_sock` is connected.

It also protects `socket.getsockname` against `OSError` in `_SelectorTransport`. This might happen on Windows if the socket is not connected (e.g. for UDP broadcasting).

https://bugs.python.org/issue31922
(cherry picked from commit 63deaa5)

Co-authored-by: Vincent Michel <[email protected]>
jaraco pushed a commit that referenced this pull request Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants