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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
May 28, 2020
Merged

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

merged 1 commit into from
May 28, 2020

Conversation

vstinner
Copy link
Member

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

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
Copy link
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

ajdavis
ajdavis suggested changes May 18, 2020
View reviewed changes
Modules/socketmodule.c Outdated
#else
#define ACQUIRE_GETADDRINFO_LOCK
#define RELEASE_GETADDRINFO_LOCK
# include <sys/param.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice we use #if defined(__OpenBSD__) above this include, apparently that's not a problem. We no longer use __FreeBSD_version or __NetBSD_Version__, so I suspect the comment and include can be deleted now too?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. That would be unrelated to this PR, so open a separated issue/PR if you want to update a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is part of this PR, since the code deleted in this PR was the last code that used the preprocessor defines from sys/param.h. This comment and include were correct before, but now that lines 203+ are deleted, this comment and include should also be deleted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh ok, now I understood your comment, ok. It seems like you are right, so I also removed the include.

@vstinner
Copy link
Member Author

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

@vstinner
Copy link
Member Author

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

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
Copy link
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
Copy link
Contributor

@ronaldoussoren ronaldoussoren left a comment

Choose a reason for hiding this comment

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

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.

Misc/NEWS.d/next/Library/2020-05-18-15-38-25.bpo-25920.PxrLY8.rst Outdated
@@ -0,0 +1,5 @@
On macOS, :func:`socket.getaddrinfo` no longer uses an internal lock to
Copy link
Contributor

Choose a reason for hiding this comment

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

The patch also removes the lock for FreeBSD, OpenBSD en NetBSD. Also note that the lock is only used when Python is build for an old enough version of these operating systems. In particular, the removed code was already not used in the installers for macOS on Python.org.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the NEWS entry to mention that python.org installer wasn't affected, and mention NetBSD, OpenBSD and FreeBSD. Does the updated NEWS entry look better?

ned-deily
ned-deily reviewed May 27, 2020
View reviewed changes
Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

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
Copy link
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 ned-deily left review comments

@ajdavis ajdavis ajdavis approved these changes

@ronaldoussoren ronaldoussoren ronaldoussoren approved these changes

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