GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-25920: Remove socket.getaddrinfo() lock on macOS by vstinner · Pull Request #20177 · python/cpython

Conversation

@vstinner

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

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 18, 2020
@bedevere-bot bedevere-bot added the awaiting core review label May 18, 2020
@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ajdavis
ajdavis suggested changes May 18, 2020
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2020-05-18-15-38-25.bpo-25920.PxrLY8.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/socketmodule.c Outdated

@ajdavis ajdavis May 18, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner May 18, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ajdavis ajdavis May 18, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner May 18, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ajdavis reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ajdavis
ajdavis approved these changes May 18, 2020
View reviewed changes
@vstinner

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

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ronaldoussoren
ronaldoussoren approved these changes May 27, 2020
View reviewed changes

@ronaldoussoren ronaldoussoren left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/NEWS.d/next/Library/2020-05-18-15-38-25.bpo-25920.PxrLY8.rst Outdated

@ronaldoussoren ronaldoussoren May 27, 2020

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@vstinner vstinner May 27, 2020

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels May 27, 2020
ned-deily
ned-deily reviewed May 27, 2020
View reviewed changes

@ned-deily ned-deily left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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()?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner
vstinner merged commit 0de437d into python:master May 28, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label 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
_scproxy and Python Launcher fail to build wtih GNU gcc on macOS 10.13+ #70505
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
@ronaldoussoren ronaldoussoren ronaldoussoren approved these changes
+1 more reviewer
@ajdavis ajdavis ajdavis approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@vstinner @ajdavis @ronaldoussoren @ned-deily @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.