GIL not released around getaddrinfo()

[jvr]

BPO	731644
Nosy	@loewis, @mhammond
Files	getaddrinfo_gil.patch: getaddrinfo vs. the GIL path Support "bpo-" in Misc/NEWS #1 getaddrinfo_gil.patch: getaddrinfo vs. the GIL path Rename README to README.rst and enhance formatting #2

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2003-05-09.07:54:12.000>
created_at = <Date 2003-05-02.23:23:59.000>
labels = ['extension-modules']
title = 'GIL not released around getaddrinfo()'
updated_at = <Date 2003-05-09.07:54:12.000>
user = 'https://bugs.python.org/jvr'

bugs.python.org fields:

activity = <Date 2003-05-09.07:54:12.000>
actor = 'jvr'
assignee = 'jvr'
closed = True
closed_date = None
closer = None
components = ['Extension Modules']
creation = <Date 2003-05-02.23:23:59.000>
creator = 'jvr'
dependencies = []
files = ['872', '873']
hgrepos = []
issue_num = 731644
keywords = []
message_count = 13.0
messages = ['15843', '15844', '15845', '15846', '15847', '15848', '15849', '15850', '15851', '15852', '15853', '15854', '15855']
nosy_count = 3.0
nosy_names = ['loewis', 'mhammond', 'jvr']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue731644'
versions = []

[jvr]

I have a situation where sock.connect() blocks in getaddrinfo(), but
since the GIL isn't released around getaddrinfo() calls, other threads
also block.

[loewis]

Logged In: YES
user_id=21627

This is not a bug. getaddrinfo isn't thread-safe on some
systems, so we must protect it from multiple access.

[jvr]

Logged In: YES
user_id=92689

Isn't that an orthogonal issue? In that we should then use a lock for
getaddrinfo(), yet do release the GIL.

I find it unacceptable that my GUI main thread can hang for several seconds
(or more) just by doing a sock.connect() in a different thread.

Short of that, can we find out on _which_ platforms getaddrinfo() isn't
thread-safe and work around that?

[loewis]

Logged In: YES
user_id=21627

See python.org/sf/604210.

Yes, using a separate lock would be possible. At a minimum,
the BSD implementation and the getaddrinfo emulation are not
thread-safe; there may be more.

As I write in 604210: it would be fine if a separate lock is
used only on the known-bad platforms, since the RFC states
getaddrinfo is thread-safe.

Would you like to work on a patch?

[mhammond]

Logged In: YES
user_id=14198

How about simply not releasing the GIL on these platforms?
Seems simpler than a new lock.

[jvr]

Logged In: YES
user_id=92689

Since my platform is BSD-ish, I want a lock ;-)

Yes, I would like to work on a patch, except I will need some hand-holding,
as I've never done anything with threads from C. Can you point me to
examples of such a lock in the Python source code?

[jvr]

Logged In: YES
user_id=92689

(Never mind, I found plenty of examples in sockemodule.c itself.)

[loewis]

Logged In: YES
user_id=21627

Mark: the original complaint came from BSD users. So if we
only release the GIL on a "good" platform, the BSD users
aren't helped. Likewise, unless compiled by VC7.x, Windows
users are not helped, since Python would use the getaddrinfo
emulation. So if this as attempted, I think we need a
best-effort solution for all systems.

You could look at the Tcl lock. I recommend you look at the
2.2 sources, since the Tcl lock logic got more complicated
recently (as Tcl now also has a multi-threaded mode of
operation).

If your platform is Mac OS X, don't trust that it is too
BSDish in this respect. If both FreeBSD and NeXT had some
function, Apple apparently has always chosen the NeXT
version. getaddrinfo was one such case. But then, it appears
that the NeXT version is even worse than the FreeBSD version.

[jvr]

Logged In: YES
user_id=92689

I've attached a first cut of a patch. It implements wrappers for getaddrinfo
and getnameinfo, to take the burden of dealing with the lock and the GIL
away from the caller. As suggested by MvL in prvt mail, I've simply reused
the existing gethostbyname lock, but renamed it to netdb_lock.

What needs to be reviewed carefully is when exactly USE_NETDB_LOCK
(formerly known as USE_GETHOSTBYNAME_LOCK) is defined. It happens to
be defined for my platform, so it "works for me".

[loewis]

Logged In: YES
user_id=21627

The patch is incorrect: usage of locked getaddrinfo depends
on whether gethostbyname_r is supported on the platform.
However, this is unrelated: A platform may have
gethostbyname_r, and still not provide a thread-safe
getaddrinfo, or it may not have gethostbyname_r, and yet
provide a thread-safe getaddrinfo.

I suggest you separate issues:

1. Should the lock variable be declared and initialized?
   You have two options: a) only create if needed (see below),
   or b) always create if Python is threaded
2. Should getaddrinfo use the lock?
   In general, it shouldn't, except for a few known-bad
   systems.
3. Should gethostbyname use the lock? Yes,
   unless the reentrant versions of this function are used.
   Notice that for Windows, gethostbyname itself is
   thread-safe,
   as it allocates a thread-local hostent.

I think the lock management code is also incorrect: There is
a certain moment after getaddrinfo returns where neither the
GIL nor the getaddrinfo lock is held, yet Python accesses
the getaddrinfo result afterwards. If getaddrinfo returns a
pointer to global data, a second call could modify the
result even though we have not processed it yet.

I'm not sure how to solve this. It is probably safest to
keep the getaddrinfo lock until freeaddrinfo has been called.

Also, for systems that don't lock getaddrinfo, I would
prefer if py_getaddrinfo was a define for getaddrinfo.

[jvr]

Logged In: YES
user_id=92689

Here's a new attempt. Martin and I went back and forth a few times, and
what I now ended up with isn't all _that_ different from the initial patch,
except it no longer uses wrapped functions.

[loewis]

Logged In: YES
user_id=21627

The patch looks fine, and works on Linux for me. Please
apply it.

[jvr]

Logged In: YES
user_id=92689

Thanks! Applied as socketmodule.c rev. 1.265.