Page MenuHomePhabricator

remotefilelog: open pack files with O_CLOEXEC
ClosedPublic

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

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
This revision was automatically updated to reflect the committed changes.