X Tutup
Skip to content

Commit 625aaf5

Browse files
asottileasottile
committed
Limit repository creation to one process
1 parent bba711f commit 625aaf5

File tree

6 files changed

+136
-49
lines changed

6 files changed

+136
-49
lines changed

‎pre_commit/file_lock.py‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
import contextlib
2+
import errno
3+
4+
5+
try: # pragma: no cover (windows)
6+
import msvcrt
7+
8+
# https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/locking
9+
10+
# on windows we lock "regions" of files, we don't care about the actual
11+
# byte region so we'll just pick *some* number here.
12+
_region = 0xffff
13+
14+
@contextlib.contextmanager
15+
def _locked(fileno):
16+
while True:
17+
try:
18+
msvcrt.locking(fileno, msvcrt.LK_LOCK, _region)
19+
except OSError as e:
20+
# Locking violation. Returned when the _LK_LOCK or _LK_RLCK
21+
# flag is specified and the file cannot be locked after 10
22+
# attempts.
23+
if e.errno != errno.EDEADLOCK:
24+
raise
25+
else:
26+
break
27+
28+
try:
29+
yield
30+
finally:
31+
# From cursory testing, it seems to get unlocked when the file is
32+
# closed so this may not be necessary.
33+
# The documentation however states:
34+
# "Regions should be locked only briefly and should be unlocked
35+
# before closing a file or exiting the program."
36+
msvcrt.locking(fileno, msvcrt.LK_UNLCK, _region)
37+
except ImportError: # pragma: no cover (posix)
38+
import fcntl
39+
40+
@contextlib.contextmanager
41+
def _locked(fileno):
42+
fcntl.flock(fileno, fcntl.LOCK_EX)
43+
try:
44+
yield
45+
finally:
46+
fcntl.flock(fileno, fcntl.LOCK_UN)
47+
48+
49+
@contextlib.contextmanager
50+
def lock(path):
51+
with open(path, 'a+') as f:
52+
with _locked(f.fileno()):
53+
yield

‎pre_commit/repository.py‎

Lines changed: 27 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -64,34 +64,42 @@ def _installed(cmd_runner, language_name, language_version, additional_deps):
6464
)
6565

6666

67-
def _install_all(venvs, repo_url):
67+
def _install_all(venvs, repo_url, store):
6868
"""Tuple of (cmd_runner, language, version, deps)"""
69-
need_installed = tuple(
70-
(cmd_runner, language_name, version, deps)
71-
for cmd_runner, language_name, version, deps in venvs
72-
if not _installed(cmd_runner, language_name, version, deps)
73-
)
69+
def _need_installed():
70+
return tuple(
71+
(cmd_runner, language_name, version, deps)
72+
for cmd_runner, language_name, version, deps in venvs
73+
if not _installed(cmd_runner, language_name, version, deps)
74+
)
75+
76+
if not _need_installed():
77+
return
78+
with store.exclusive_lock():
79+
# Another process may have already completed this work
80+
need_installed = _need_installed()
81+
if not need_installed: # pragma: no cover (race)
82+
return
7483

75-
if need_installed:
7684
logger.info(
7785
'Installing environment for {}.'.format(repo_url),
7886
)
7987
logger.info('Once installed this environment will be reused.')
8088
logger.info('This may take a few minutes...')
8189

82-
for cmd_runner, language_name, version, deps in need_installed:
83-
language = languages[language_name]
84-
venv = environment_dir(language.ENVIRONMENT_DIR, version)
90+
for cmd_runner, language_name, version, deps in need_installed:
91+
language = languages[language_name]
92+
venv = environment_dir(language.ENVIRONMENT_DIR, version)
8593

86-
# There's potentially incomplete cleanup from previous runs
87-
# Clean it up!
88-
if cmd_runner.exists(venv):
89-
shutil.rmtree(cmd_runner.path(venv))
94+
# There's potentially incomplete cleanup from previous runs
95+
# Clean it up!
96+
if cmd_runner.exists(venv):
97+
shutil.rmtree(cmd_runner.path(venv))
9098

91-
language.install_environment(cmd_runner, version, deps)
92-
# Write our state to indicate we're installed
93-
state = _state(deps)
94-
_write_state(cmd_runner, venv, state)
99+
language.install_environment(cmd_runner, version, deps)
100+
# Write our state to indicate we're installed
101+
state = _state(deps)
102+
_write_state(cmd_runner, venv, state)
95103

96104

97105
def _validate_minimum_version(hook):
@@ -174,7 +182,7 @@ def _venvs(self):
174182

175183
def require_installed(self):
176184
if not self.__installed:
177-
_install_all(self._venvs, self.repo_config['repo'])
185+
_install_all(self._venvs, self.repo_config['repo'], self.store)
178186
self.__installed = True
179187

180188
def run_hook(self, hook, file_args):

‎pre_commit/store.py‎

Lines changed: 48 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from cached_property import cached_property
1111

1212
import pre_commit.constants as C
13+
from pre_commit import file_lock
1314
from pre_commit.prefixed_command_runner import PrefixedCommandRunner
1415
from pre_commit.util import clean_path_on_failure
1516
from pre_commit.util import cmd_output
@@ -37,13 +38,20 @@ def _get_default_directory():
3738

3839
class Store(object):
3940
get_default_directory = staticmethod(_get_default_directory)
41+
__created = False
4042

4143
def __init__(self, directory=None):
4244
if directory is None:
4345
directory = self.get_default_directory()
4446

4547
self.directory = directory
46-
self.__created = False
48+
49+
@contextlib.contextmanager
50+
def exclusive_lock(self, quiet=False):
51+
if not quiet:
52+
logger.info('Locking pre-commit directory')
53+
with file_lock.lock(os.path.join(self.directory, '.lock')):
54+
yield
4755

4856
def _write_readme(self):
4957
with io.open(os.path.join(self.directory, 'README'), 'w') as readme:
@@ -75,12 +83,17 @@ def _write_sqlite_db(self):
7583
os.rename(tmpfile, self.db_path)
7684

7785
def _create(self):
78-
if os.path.exists(self.db_path):
79-
return
8086
if not os.path.exists(self.directory):
8187
os.makedirs(self.directory)
8288
self._write_readme()
83-
self._write_sqlite_db()
89+
90+
if os.path.exists(self.db_path):
91+
return
92+
with self.exclusive_lock(quiet=True):
93+
# Another process may have already completed this work
94+
if os.path.exists(self.db_path): # pragma: no cover (race)
95+
return
96+
self._write_sqlite_db()
8497

8598
def require_created(self):
8699
"""Require the pre-commit file store to be created."""
@@ -91,27 +104,37 @@ def require_created(self):
91104
def _new_repo(self, repo, ref, make_strategy):
92105
self.require_created()
93106

94-
# Check if we already exist
95-
with sqlite3.connect(self.db_path) as db:
96-
result = db.execute(
97-
'SELECT path FROM repos WHERE repo = ? AND ref = ?',
98-
[repo, ref],
99-
).fetchone()
100-
if result:
101-
return result[0]
102-
103-
logger.info('Initializing environment for {}.'.format(repo))
104-
105-
directory = tempfile.mkdtemp(prefix='repo', dir=self.directory)
106-
with clean_path_on_failure(directory):
107-
make_strategy(directory)
108-
109-
# Update our db with the created repo
110-
with sqlite3.connect(self.db_path) as db:
111-
db.execute(
112-
'INSERT INTO repos (repo, ref, path) VALUES (?, ?, ?)',
113-
[repo, ref, directory],
114-
)
107+
def _get_result():
108+
# Check if we already exist
109+
with sqlite3.connect(self.db_path) as db:
110+
result = db.execute(
111+
'SELECT path FROM repos WHERE repo = ? AND ref = ?',
112+
[repo, ref],
113+
).fetchone()
114+
if result:
115+
return result[0]
116+
117+
result = _get_result()
118+
if result:
119+
return result
120+
with self.exclusive_lock():
121+
# Another process may have already completed this work
122+
result = _get_result()
123+
if result: # pragma: no cover (race)
124+
return result
125+
126+
logger.info('Initializing environment for {}.'.format(repo))
127+
128+
directory = tempfile.mkdtemp(prefix='repo', dir=self.directory)
129+
with clean_path_on_failure(directory):
130+
make_strategy(directory)
131+
132+
# Update our db with the created repo
133+
with sqlite3.connect(self.db_path) as db:
134+
db.execute(
135+
'INSERT INTO repos (repo, ref, path) VALUES (?, ?, ?)',
136+
[repo, ref, directory],
137+
)
115138
return directory
116139

117140
def clone(self, repo, ref):

‎tests/commands/install_uninstall_test.py‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,8 @@ def _get_commit_output(tempdir_factory, touch_file='foo', **kwargs):
142142

143143

144144
NORMAL_PRE_COMMIT_RUN = re.compile(
145-
r'^\[INFO\] Initializing environment for .+\.\r?\n'
145+
r'^\[INFO\] Locking pre-commit directory\r?\n'
146+
r'\[INFO\] Initializing environment for .+\.\r?\n'
146147
r'Bash hook\.+Passed\r?\n'
147148
r'\[master [a-f0-9]{7}\] Commit!\r?\n' +
148149
FILES_CHANGED +
@@ -254,7 +255,8 @@ def test_environment_not_sourced(tempdir_factory):
254255

255256

256257
FAILING_PRE_COMMIT_RUN = re.compile(
257-
r'^\[INFO\] Initializing environment for .+\.\r?\n'
258+
r'^\[INFO\] Locking pre-commit directory\r?\n'
259+
r'\[INFO\] Initializing environment for .+\.\r?\n'
258260
r'Failing hook\.+Failed\r?\n'
259261
r'hookid: failing_hook\r?\n'
260262
r'\r?\n'
@@ -332,6 +334,7 @@ def test_install_existing_hook_no_overwrite_idempotent(tempdir_factory):
332334

333335
FAIL_OLD_HOOK = re.compile(
334336
r'fail!\r?\n'
337+
r'\[INFO\] Locking pre-commit directory\r?\n'
335338
r'\[INFO\] Initializing environment for .+\.\r?\n'
336339
r'Bash hook\.+Passed\r?\n',
337340
)

‎tests/repository_test.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -547,8 +547,8 @@ def test_reinstall(tempdir_factory, store, log_info_mock):
547547
config = make_config_from_repo(path)
548548
repo = Repository.create(config, store)
549549
repo.require_installed()
550-
# We print some logging during clone (1) + install (3)
551-
assert log_info_mock.call_count == 4
550+
# We print some logging during clone (2) + install (4)
551+
assert log_info_mock.call_count == 6
552552
log_info_mock.reset_mock()
553553
# Reinstall with same repo should not trigger another install
554554
repo.require_installed()

‎tests/store_test.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ def test_clone(store, tempdir_factory, log_info_mock):
8888

8989
ret = store.clone(path, sha)
9090
# Should have printed some stuff
91-
assert log_info_mock.call_args_list[0][0][0].startswith(
91+
assert log_info_mock.call_args_list[1][0][0].startswith(
9292
'Initializing environment for ',
9393
)
9494

0 commit comments

Comments
 (0)
X Tutup