Skip to content

Commit

Permalink
Avoid redundant work while hackily_snapshot()ing. (pantsbuild#7829)
Browse files Browse the repository at this point in the history
### Problem

In runs with high degrees of concurrency, we end up seeing many simultaneous instances of tools being fetched and snapshotted.

### Solution

Enforce a single fetch+snapshot attempt per tool. As mentioned in the comment, this is a temporary solution until pantsbuild#7790 migrates this into the engine itself.
  • Loading branch information
stuhood authored Jun 1, 2019
1 parent 1dd84a6 commit d3c9311
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 1 deletion.
19 changes: 18 additions & 1 deletion src/python/pants/binaries/binary_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import logging
import os
import threading
from builtins import str

from future.utils import text_type
Expand Down Expand Up @@ -50,6 +51,10 @@ class BinaryToolBase(Subsystem):
# Subclasses may set this to provide extra register() kwargs for the --version option.
extra_version_option_kwargs = None

def __init__(self, *args, **kwargs):
super(BinaryToolBase, self).__init__(*args, **kwargs)
self._snapshot_lock = threading.Lock()

@classmethod
def subsystem_dependencies(cls):
sub_deps = super(BinaryToolBase, cls).subsystem_dependencies() + (BinaryUtil.Factory,)
Expand Down Expand Up @@ -179,7 +184,8 @@ def _select_for_version(self, version):
binary_request = self._make_binary_request(version)
return self._binary_util.select(binary_request)

def hackily_snapshot(self, context):
@memoized_method
def _hackily_snapshot_exclusive(self, context):
bootstrapdir = self.get_options().pants_bootstrapdir
relpath = os.path.relpath(self.select(context), bootstrapdir)
snapshot = context._scheduler.capture_snapshots((
Expand All @@ -190,6 +196,17 @@ def hackily_snapshot(self, context):
))[0]
return (relpath, snapshot)

def hackily_snapshot(self, context):
"""Returns a Snapshot of this tool after downloading it.
TODO: See https://github.com/pantsbuild/pants/issues/7790, which would make this unnecessary
due to the engine's memoization and caching.
"""
# We call a memoized method under a lock in order to avoid doing a bunch of redundant
# fetching and snapshotting.
with self._snapshot_lock:
return self._hackily_snapshot_exclusive(context)


class NativeTool(BinaryToolBase):
"""A base class for native-code tools.
Expand Down
25 changes: 25 additions & 0 deletions tests/python/pants_test/binaries/test_binary_tool.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@

from __future__ import absolute_import, division, print_function, unicode_literals

import os
from builtins import str

from pants.binaries.binary_tool import BinaryToolBase
from pants.binaries.binary_util import (BinaryToolFetcher, BinaryToolUrlGenerator, BinaryUtil,
HostPlatform)
from pants.option.scope import GLOBAL_SCOPE
from pants.util.contextutil import temporary_dir
from pants.util.dirutil import safe_file_dump
from pants_test.test_base import TestBase


Expand Down Expand Up @@ -82,6 +85,7 @@ def setUp(self):
options={
GLOBAL_SCOPE: {
'binaries_baseurls': ['https://binaries.example.org'],
'pants_bootstrapdir': str(temporary_dir()),
},
'another-tool': {
'version': '0.0.2',
Expand Down Expand Up @@ -144,3 +148,24 @@ def test_urls(self):
self.assertIn(
"Failed to fetch binary from https://custom-url.example.org/files/custom_urls_tool-v2.3-zzz-alternate:",
err_msg)

def test_hackily_snapshot(self):
with temporary_dir() as temp_dir:
safe_file_dump(
os.path.join(temp_dir, 'bin', DefaultVersion.name, DefaultVersion.default_version, DefaultVersion.name),
'content!',
)
context = self.context(
for_subsystems=[DefaultVersion],
options={
GLOBAL_SCOPE: {
'binaries_baseurls': ['file:///{}'.format(temp_dir)],
},
})
self.maxDiff = None
default_version_tool = DefaultVersion.global_instance()
_, snapshot = default_version_tool.hackily_snapshot(context)
self.assertEqual(
'51a98706ab7458069aabe01856cb352ca97686e3edd3bf9ebd3205c2b38b2974',
snapshot.directory_digest.fingerprint
)

0 comments on commit d3c9311

Please sign in to comment.