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

Do not connect to any sockets on import #5808

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

fjetter
Copy link
Member

@fjetter fjetter commented Feb 15, 2022

Supersedes #5744

IMO the above linked PR is not only about the warning filter applied but my bigger concern is that the get_ip call connects/initializes a socket on import. While we do not attempt to connect to any remote since it's a UDP socket it is still a kernel interaction, a socket allocation, etc.

This is typically very fast but also very unnecessary, particularly since we're usually not using the inproc backend.

With this change, there is no get_ip called at import time. No sane way to test this. If there was a way to ensure a test is the very first one executed we could check that the cache of _get_ip is empty but even if that's possible, it's hardly worth the effort

https://man7.org/linux/man-pages/man7/udp.7.html

When a UDP socket is created, its local and remote addresses are
unspecified. Datagrams can be sent immediately using sendto(2)
or sendmsg(2) with a valid destination address as an argument.
When connect(2) is called on the socket, the default destination
address is set and datagrams can now be sent using send(2) or
write(2) without specifying a destination address.

@graingert
Copy link
Member

With this change, there is no get_ip called at import time. No sane way to test this. If there was a way to ensure a test is the very first one executed we could check that the cache of _get_ip is empty but even if that's possible, it's hardly worth the effort

is there a reason for using @toolz.memoise over @functools.lru_cache (python 2 support) ? lru_cache can be cleared in tests: pytoolz/toolz#228

@fjetter
Copy link
Member Author

fjetter commented Feb 15, 2022

is there a reason for using @toolz.memoise over @functools.lru_cache (python 2 support) ? lru_cache can be cleared in tests: pytoolz/toolz#228

Probably no reason but right now I do not want to find out if there is any. If anything, this should be a dedicated PR but for now I don't see a reason to change this other than aesthetics

@github-actions
Copy link
Contributor

Unit Test Results

       18 files  ±0         18 suites  ±0   10h 36m 13s ⏱️ - 14m 30s
  2 596 tests ±0    2 517 ✔️ +  3       79 💤 ±  0  0  - 3 
23 230 runs  ±0  21 731 ✔️  - 86  1 499 💤 +89  0  - 3 

Results for commit 59923e2. ± Comparison against base commit d3f3012.

@fjetter fjetter merged commit 0efad51 into dask:main Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants