From 144c23efbb4e9565debad03c13c5bcab833a8336 Mon Sep 17 00:00:00 2001 From: Zac Medico Date: Tue, 27 Mar 2012 11:28:12 -0700 Subject: [PATCH] Close fewer file descriptors for fork / no exec. This will fix bug #374335. --- pym/_emerge/EbuildFetcher.py | 14 ++++---------- pym/portage/dbapi/_MergeProcess.py | 13 +++---------- pym/portage/locks.py | 23 ++++++++++++++++++++++- pym/portage/process.py | 14 +++++++++++++- 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 -- 2.26.2