◐ Shell
reader mode source ↗
Skip to content

bpo-25920: Remove socket.getaddrinfo() lock on macOS#20177

Merged
vstinner merged 1 commit into
python:masterfrom
vstinner:remove_getaddrinfo_lock
May 28, 2020
Merged

bpo-25920: Remove socket.getaddrinfo() lock on macOS#20177
vstinner merged 1 commit into
python:masterfrom
vstinner:remove_getaddrinfo_lock

Conversation

@vstinner

@vstinner vstinner commented May 18, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

On macOS, socket.getaddrinfo() no longer uses an internal lock to
prevenet race conditions when calling getaddrinfo(). getaddrinfo is
thread-safe is macOS 10.5, whereas Python 3.9 requires macOS 10.6 or
newer.

The lock was also used on FreeBSD older than 5.3, OpenBSD older than
201311 and NetBSD older than 4.

https://bugs.python.org/issue25920

@vstinner

vstinner commented May 18, 2020

Copy link
Copy Markdown
Member Author

cc @ronaldoussoren @ned-deily

This change also impacts socket.getnameinfo() and setipaddr() helper which is used by getsockaddrarg() to parse a "socket address": sock.bind, sock.connect, sock.connect_ex, sock.sendto, sock.sendmsg. I'm not sure if it's worth it to list all impacted methods.

With this change, socketmodule.c still uses netdb_lock on platforms which don't provide gethostbyname_r():

#if !defined(HAVE_GETHOSTBYNAME_R) && !defined(MS_WINDOWS)
# define USE_GETHOSTBYNAME_LOCK
#endif

@vstinner

vstinner commented May 18, 2020

Copy link
Copy Markdown
Member Author

PR rebased to amend the commit messaged to a fix a typo (see previous review comments).

@vstinner

vstinner commented May 18, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member Author

I tested manually my PR on FreeBSD: it works as expected, I can still build Python and test_socket pass.

But I had to revert commit 0da5466: it's a regression, see bpo-26317.

@vstinner

vstinner commented May 26, 2020

Copy link
Copy Markdown
Member Author

@ronaldoussoren @ned-deily: I plan to merge this PR into master (but not backport it to 3.9) at the end of the week. Do you plan to review it?

ronaldoussoren
ronaldoussoren approved these changes May 27, 2020
View reviewed changes

@ronaldoussoren ronaldoussoren left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Patch looks good to me, other than a pedantic note for the news entry.

As an aside (not to be addressed in the PR): Apparently gethostbyname() and related functions are thread-safe on macOS. This is according to the manpage on macOS 10.15. I haven't checked in which version that changed. This allows avoiding the use of the gethostbyname lock as well.

ned-deily
ned-deily reviewed May 27, 2020
View reviewed changes

@ned-deily ned-deily left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

If @ronaldoussoren is fine with it, I'm fine with it.

@vstinner
bpo-25920: Remove socket.getaddrinfo() lock on macOS …
1275504 
On macOS, socket.getaddrinfo() no longer uses an internal lock to
prevent race conditions when calling getaddrinfo(). getaddrinfo is
thread-safe is macOS 10.5, whereas Python 3.9 requires macOS 10.6 or
newer.

The lock was also used on FreeBSD older than 5.3, OpenBSD older than
201311 and NetBSD older than 4.
@vstinner

vstinner commented May 27, 2020

Copy link
Copy Markdown
Member Author

As an aside (not to be addressed in the PR): Apparently gethostbyname() and related functions are thread-safe on macOS. This is according to the manpage on macOS 10.15. I haven't checked in which version that changed. This allows avoiding the use of the gethostbyname lock as well.

I chose to restrict my PR to the socket.getaddrinfo() lock on purpose:
#20177 (comment)

Would you mind to open a separated issue for gethostbyname()?

@vstinner vstinner merged commit 0de437d into python:master May 28, 2020
@vstinner vstinner deleted the remove_getaddrinfo_lock branch May 28, 2020 15:23
CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request May 30, 2020
@CuriousLearner
Merge branch 'master' of github.com:python/cpython into bpo-26543 …
d3d7100 
* 'master' of github.com:python/cpython: (497 commits)
  bpo-40061: Fix a possible refleak in _asynciomodule.c (pythonGH-19748)
  bpo-40798: Generate a different message for already removed elements (pythonGH-20483)
  closes bpo-29017: Update the bindings for Qt information with PySide2 (pythonGH-20149)
  bpo-39885: Make IDLE context menu cut and copy work again (pythonGH-18951)
  bpo-29882: Add an efficient popcount method for integers (python#771)
  Further de-linting of zoneinfo module (python#20499)
  bpo-40780: Fix failure of _Py_dg_dtoa to remove trailing zeros (pythonGH-20435)
  Indicate that abs() method accept argument that implement __abs__(), just like call() method in the docs (pythonGH-20509)
  bpo-39040: Fix parsing of email mime headers with whitespace between encoded-words. (pythongh-17620)
  bpo-40784: Fix sqlite3 deterministic test (pythonGH-20448)
  bpo-30064: Properly skip unstable loop.sock_connect() racing test (pythonGH-20494)
  Note the output ordering of combinatoric functions (pythonGH-19732)
  bpo-40474: Updated coverage.yml to better report coverage stats (python#19851)
  bpo-40806: Clarify that itertools.product immediately consumes its inpt (pythonGH-20492)
  bpo-1294959: Try to clarify the meaning of platlibdir (pythonGH-20332)
  bpo-37878: PyThreadState_DeleteCurrent() was not removed (pythonGH-20489)
  bpo-40777: Initialize PyDateTime_IsoCalendarDateType.tp_base at run-time (pythonGH-20493)
  bpo-40755: Add missing multiset operations to Counter() (pythonGH-20339)
  bpo-25920: Remove socket.getaddrinfo() lock on macOS (pythonGH-20177)
  bpo-40275: Fix test.support.threading_helper (pythonGH-20488)
  ...
@RobertPFischer RobertPFischer mannequin mentioned this pull request May 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ned-deily ned-deily

@ronaldoussoren ronaldoussoren ronaldoussoren approved these changes

+1 more reviewer

@ajdavis ajdavis ajdavis approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vstinner @ajdavis @ronaldoussoren @ned-deily @the-knights-who-say-ni @bedevere-bot