bpo-25920: Remove socket.getaddrinfo() lock on macOS by vstinner · Pull Request #20177 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

vstinner

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

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

#else
#define ACQUIRE_GETADDRINFO_LOCK
#define RELEASE_GETADDRINFO_LOCK
# include <sys/param.h>

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?

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.

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.

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

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

ajdavis

@vstinner

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

@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

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.

@@ -0,0 +1,5 @@
On macOS, :func:`socket.getaddrinfo` no longer uses an internal lock to

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.

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

Choose a reason for hiding this comment

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

@vstinner

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

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