Skip to content

Commit

Permalink
Review feedback and a few test fixes.
Browse files Browse the repository at this point in the history
  • Loading branch information
stuhood committed Mar 15, 2018
1 parent 6b5a9a4 commit e49fd02
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 21 deletions.
1 change: 1 addition & 0 deletions src/python/pants/engine/scheduler.py
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ def to_keys(self, subjects):
return list(self._to_key(subject) for subject in subjects)

def with_fork_context(self, func):
"""See the rustdocs for `scheduler_fork_context` for more information."""
res = self._native.lib.scheduler_fork_context(self._scheduler, Function(self._to_key(func)))
return self._from_py_result(res)

Expand Down
37 changes: 19 additions & 18 deletions src/python/pants/pantsd/process_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from __future__ import (absolute_import, division, generators, nested_scopes, print_function,
unicode_literals, with_statement)

import functools
import logging
import os
import signal
Expand Down Expand Up @@ -401,14 +402,6 @@ def _kill(self, kill_sig):
if self.pid:
os.kill(self.pid, kill_sig)

def _noop_fork_context(self, func):
"""A "fork context" without any special behaviour.
A fork context is a context in which it is safe to call `fork`. This is not a contextmanager
because that would make interacting with native code more challenging.
"""
return func()

def terminate(self, signal_chain=KILL_CHAIN, kill_wait=KILL_WAIT_SEC, purge=True):
"""Ensure a process is terminated by sending a chain of kill signals (SIGTERM, SIGKILL)."""
alive = self.is_alive()
Expand Down Expand Up @@ -455,8 +448,12 @@ def daemonize(self, pre_fork_opts=None, post_fork_parent_opts=None, post_fork_ch
below) due to the fact that the daemons that pants would run are typically personal user
daemons. Having a disparate umask from pre-vs-post fork causes files written in each phase to
differ in their permissions without good reason - in this case, we want to inherit the umask.
:param fork_context: A function which accepts and calls a function that will call fork. This
is not a contextmanager/generator because that would make interacting with native code more
challenging. If no fork_context is passed, the fork function is called directly.
"""
fork_context = fork_context or self._noop_fork_context


def double_fork():
logger.debug('forking %s', self)
Expand All @@ -465,28 +462,32 @@ def double_fork():
os.setsid()
second_pid = os.fork()
if second_pid == 0:
return False
return False, True
else:
if write_pid: self.write_pid(second_pid)
return True
return True, False
else:
# This prevents un-reaped, throw-away parent processes from lingering in the process table.
os.waitpid(pid, 0)
return None
return False, False

fork_func = functools.partial(fork_context, double_fork) or double_fork

# Perform the double fork under the fork_context. Three outcomes are possible after the double
# fork: we're either the original process, the double-fork parent, or the double-fork child.
# These are represented by parent_or_child being None, True, or False, respectively.
# Perform the double fork (optionally under the fork_context). Three outcomes are possible after
# the double fork: we're either the original process, the double-fork parent, or the double-fork
# child. We assert below that a process is not somehow both the parent and the child.
self.purge_metadata()
self.pre_fork(**pre_fork_opts or {})
parent_or_child = fork_context(double_fork)
if parent_or_child is None:
is_parent, is_child = fork_func()
if not is_parent and not is_child:
return

try:
if parent_or_child:
if is_parent:
assert not is_child
self.post_fork_parent(**post_fork_parent_opts or {})
else:
assert not is_parent
os.chdir(self._buildroot)
self.post_fork_child(**post_fork_child_opts or {})
except Exception:
Expand Down
4 changes: 2 additions & 2 deletions src/python/pants/pantsd/service/store_gc_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,14 @@ def _launch_thread(f):

def _extend_lease(self):
while 1:
# Use the fork lock to ensure this thread isn't cloned via fork while holding the graph lock.
self._logger.debug('Extending leases')
self._scheduler.lease_files_in_graph()
self._logger.debug('Done extending leases')
time.sleep(self._LEASE_EXTENSION_INTERVAL_SECONDS)

def _garbage_collect(self):
while 1:
time.sleep(self._GARBAGE_COLLECTION_INTERVAL_SECONDS)
# Grab the fork lock in case lmdb internally isn't fork-without-exec-safe.
self._logger.debug('Garbage collecting store')
self._scheduler.garbage_collect_store()
self._logger.debug('Done garbage collecting store')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ def setUp(self):
BaseTest.setUp(self)
self.mock_watchman = mock.create_autospec(Watchman, spec_set=True)
self.service = FSEventService(self.mock_watchman, self.BUILD_ROOT, self.WORKER_COUNT)
self.service.setup(None, None, executor=TestExecutor())
self.service.setup(None, executor=TestExecutor())
self.service.register_all_files_handler(lambda x: True, name='test')
self.service.register_all_files_handler(lambda x: False, name='test2')

Expand Down

0 comments on commit e49fd02

Please sign in to comment.