Close fewer file descriptors for fork / no exec.

This will fix bug #374335.

4 files changed, 42 insertions, 22 deletions

diff --git a/pym/_emerge/EbuildFetcher.py b/pym/_emerge/EbuildFetcher.py
index f6dab5471..c0a7fddaa 100644
--- a/pym/_emerge/EbuildFetcher.py
+++ b/pym/_emerge/EbuildFetcher.py
@@ -1,4 +1,4 @@
-# Copyright 1999-2011 Gentoo Foundation
+# Copyright 1999-2012 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
 import traceback
@@ -6,7 +6,6 @@ import traceback
 from _emerge.SpawnProcess import SpawnProcess
 import copy
 import io
-import platform
 import signal
 import sys
 import portage
@@ -170,14 +169,9 @@ class EbuildFetcher(SpawnProcess):
 			portage.process.spawned_pids.append(pid)
 			return [pid]
 
-		# TODO: Find out why PyPy 1.8 with close_fds=True triggers
-		# "[Errno 9] Bad file descriptor" in subprocesses. It could
-		# be due to garbage collection of file objects that were not
-		# closed before going out of scope, since PyPy's garbage
-		# collector does not support the refcounting semantics that
-		# CPython does.
-		close_fds = platform.python_implementation() != 'PyPy'
-		portage.process._setup_pipes(fd_pipes, close_fds=close_fds)
+		portage.locks._close_fds()
+		# Disable close_fds since we don't exec (see _setup_pipes docstring).
+		portage.process._setup_pipes(fd_pipes, close_fds=False)
 
 		# Use default signal handlers in order to avoid problems
 		# killing subprocesses as reported in bug #353239.
diff --git a/pym/portage/dbapi/_MergeProcess.py b/pym/portage/dbapi/_MergeProcess.py
index 301625c81..b5f6a0b0b 100644
--- a/pym/portage/dbapi/_MergeProcess.py
+++ b/pym/portage/dbapi/_MergeProcess.py
@@ -2,7 +2,6 @@
 # Distributed under the terms of the GNU General Public License v2
 
 import io
-import platform
 import signal
 import sys
 import traceback
@@ -155,15 +154,9 @@ class MergeProcess(SpawnProcess):
 			return [pid]
 
 		os.close(elog_reader_fd)
-
-		# TODO: Find out why PyPy 1.8 with close_fds=True triggers
-		# "[Errno 9] Bad file descriptor" in subprocesses. It could
-		# be due to garbage collection of file objects that were not
-		# closed before going out of scope, since PyPy's garbage
-		# collector does not support the refcounting semantics that
-		# CPython does.
-		close_fds = platform.python_implementation() != 'PyPy'
-		portage.process._setup_pipes(fd_pipes, close_fds=close_fds)
+		portage.locks._close_fds()
+		# Disable close_fds since we don't exec (see _setup_pipes docstring).
+		portage.process._setup_pipes(fd_pipes, close_fds=False)
 
 		# Use default signal handlers since the ones inherited
 		# from the parent process are irrelevant here.
diff --git a/pym/portage/locks.py b/pym/portage/locks.py
index 9fee5ae5d..87fbe0726 100644
--- a/pym/portage/locks.py
+++ b/pym/portage/locks.py
@@ -1,5 +1,5 @@
 # portage: Lock management code
-# Copyright 2004-2011 Gentoo Foundation
+# Copyright 2004-2012 Gentoo Foundation
 # Distributed under the terms of the GNU General Public License v2
 
 __all__ = ["lockdir", "unlockdir", "lockfile", "unlockfile", \
@@ -36,6 +36,20 @@ if platform.python_implementation() == 'PyPy':
 # so that it doesn't interfere with the status display.
 _quiet = False
 
+
+_open_fds = set()
+
+def _close_fds():
+	"""
+	This is intended to be called after a fork, in order to close file
+	descriptors for locks held by the parent process. This can be called
+	safely after a fork without exec, unlike the _setup_pipes close_fds
+	behavior.
+	.
+	"""
+	while _open_fds:
+		os.close(_open_fds.pop())
+
 def lockdir(mydir, flags=0):
 	return lockfile(mydir, wantnewlockfile=1, flags=flags)
 def unlockdir(mylock):
@@ -193,6 +207,9 @@ def lockfile(mypath, wantnewlockfile=0, unlinkfile=0,
 			mypath, wantnewlockfile=wantnewlockfile, unlinkfile=unlinkfile,
 			waiting_msg=waiting_msg, flags=flags)
 
+	if myfd != HARDLINK_FD:
+		_open_fds.add(myfd)
+
 	writemsg(str((lockfilename,myfd,unlinkfile))+"\n",1)
 	return (lockfilename,myfd,unlinkfile,locking_method)
 
@@ -233,6 +250,7 @@ def unlockfile(mytuple):
 		writemsg(_("lockfile does not exist '%s'\n") % lockfilename,1)
 		if myfd is not None:
 			os.close(myfd)
+			_open_fds.remove(myfd)
 		return False
 
 	try:
@@ -243,6 +261,7 @@ def unlockfile(mytuple):
 	except OSError:
 		if isinstance(lockfilename, basestring):
 			os.close(myfd)
+			_open_fds.remove(myfd)
 		raise IOError(_("Failed to unlock file '%s'\n") % lockfilename)
 
 	try:
@@ -264,6 +283,7 @@ def unlockfile(mytuple):
 			else:
 				writemsg(_("lockfile does not exist '%s'\n") % lockfilename, 1)
 				os.close(myfd)
+				_open_fds.remove(myfd)
 				return False
 	except SystemExit:
 		raise
@@ -276,6 +296,7 @@ def unlockfile(mytuple):
 	# open fd closed automatically on them.
 	if isinstance(lockfilename, basestring):
 		os.close(myfd)
+		_open_fds.remove(myfd)
 
 	return True
 
diff --git a/pym/portage/process.py b/pym/portage/process.py
index 94687eaa0..f3cec8815 100644
--- a/pym/portage/process.py
+++ b/pym/portage/process.py
@@ -401,7 +401,19 @@ def _exec(binary, mycommand, opt_name, fd_pipes, env, gid, groups, uid, umask,
 	os.execve(binary, myargs, env)
 
 def _setup_pipes(fd_pipes, close_fds=True):
-	"""Setup pipes for a forked process."""
+	"""Setup pipes for a forked process.
+
+	WARNING: When not followed by exec, the close_fds behavior
+	can trigger interference from destructors that close file
+	descriptors. This interference happens when the garbage
+	collector intermittently executes such destructors after their
+	corresponding file descriptors have been re-used, leading
+	to intermittent "[Errno 9] Bad file descriptor" exceptions in
+	forked processes. This problem has been observed with PyPy 1.8,
+	and also with CPython under some circumstances (as triggered
+	by xmpppy in bug #374335). In order to close a safe subset of
+	file descriptors, see portage.locks._close_fds().
+	"""
 	my_fds = {}
 	# To protect from cases where direct assignment could
 	# clobber needed fds ({1:2, 2:1}) we first dupe the fds