This is an archive of the discontinued Mercurial Phabricator instance.

Differential D398

remotefilelog: open pack files with O_CLOEXEC
ClosedPublic

Authored by simpkins on Aug 14 2017, 10:55 PM.
Tags
None
Subscribers
None

Details

Reviewers
quark
durham
Group Reviewers
Restricted Project
Commits
rFBHGX6ee35c7861c0: remotefilelog: open pack files with O_CLOEXEC
Summary

If os.name is "posix", add the "e" flag when opening pack files. On systems
using glibc 2.7+ this causes the underlying operating system open() call to use
the O_CLOEXEC flag. This ensures that the pack file descriptors will not be
inherited by any children processes. This can be helpful since the
remotefilelog code can sometimes have thousands of pack files open. This
causes problems for children processes that expect to be able to use select()
on file descriptors they create, since they can end up with file descriptor
numbers over 1024.

While making sure to always set close_fds=True when calling subprocess.Popen()
can help with this, using O_CLOEXEC to fix the problem from the start seems
like a nicer fix.

The extra "e" flag generally should be ignored by other C library
implementations that do not support this flag. I explicitly scoped this to the
"posix" OS just to ensure that this cannot collide with other OS-specific flags
on other operating systems. (Windows also defines several of its own custom
fopen() flags. Windows does not currently use "e", but it seems best to avoid
it anyway.)

Test Plan

Ran hg under strace and confirmed that the pack files were opened with
O_CLOEXEC.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

simpkins created this revision.Aug 14 2017, 10:55 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 14 2017, 10:55 PM
quark accepted this revision.Aug 15 2017, 12:07 AM
This revision is now accepted and ready to land.Aug 15 2017, 12:07 AM
Closed by commit rFBHGX6ee35c7861c0: remotefilelog: open pack files with O_CLOEXEC (authored by simpkins). · Explain WhyAug 16 2017, 11:18 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mremotefilelog/basepack.py (13 lines)

Diff 993

remotefilelog/basepack.py

from __future__ import absolute_import from __future__ import absolute_import
import errno, hashlib, mmap, os, struct, time import errno, hashlib, mmap, os, struct, time
from mercurial import policy, util from mercurial import policy, pycompat, util
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import vfs as vfsmod from mercurial import vfs as vfsmod
from . import shallowutil from . import shallowutil
osutil = policy.importmod(r'osutil') osutil = policy.importmod(r'osutil')
# The pack version supported by this implementation. This will need to be # The pack version supported by this implementation. This will need to be
# 10 step fanout scan = 2^16 / (2^16 / 8) # fanout space divided by entries # 10 step fanout scan = 2^16 / (2^16 / 8) # fanout space divided by entries
SMALLFANOUTCUTOFF = 2**16 / 8 SMALLFANOUTCUTOFF = 2**16 / 8
# The amount of time to wait between checking for new packs. This prevents an # The amount of time to wait between checking for new packs. This prevents an
# exception when data is moved to a new pack after the process has already # exception when data is moved to a new pack after the process has already
# loaded the pack list. # loaded the pack list.
REFRESHRATE = 0.1 REFRESHRATE = 0.1
if pycompat.osname == 'posix':
# With glibc 2.7+ the 'e' flag uses O_CLOEXEC when opening.
# The 'e' flag will be ignored on older versions of glibc.
PACKOPENMODE = 'rbe'
else:
PACKOPENMODE = 'rb'
class basepackstore(object): class basepackstore(object):
def __init__(self, ui, path): def __init__(self, ui, path):
self.path = path self.path = path
self.packs = [] self.packs = []
# lastrefesh is 0 so we'll immediately check for new packs on the first # lastrefesh is 0 so we'll immediately check for new packs on the first
# failure. # failure.
self.lastrefresh = 0 self.lastrefresh = 0
SUPPORTED_VERSIONS = [0] SUPPORTED_VERSIONS = [0]
def __init__(self, path): def __init__(self, path):
self.path = path self.path = path
self.packpath = path + self.PACKSUFFIX self.packpath = path + self.PACKSUFFIX
self.indexpath = path + self.INDEXSUFFIX self.indexpath = path + self.INDEXSUFFIX
# TODO: use an opener/vfs to access these paths # TODO: use an opener/vfs to access these paths
self.indexfp = open(self.indexpath, 'rb') self.indexfp = open(self.indexpath, PACKOPENMODE)
self.datafp = open(self.packpath, 'rb') self.datafp = open(self.packpath, PACKOPENMODE)
self.indexsize = os.stat(self.indexpath).st_size self.indexsize = os.stat(self.indexpath).st_size
self.datasize = os.stat(self.packpath).st_size self.datasize = os.stat(self.packpath).st_size
self._index = None self._index = None
self._data = None self._data = None
self.freememory() # initialize the mmap self.freememory() # initialize the mmap