From ff27b6ed422ed832f959bf6ca57e07afc9f3f5f4 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Mon, 24 Apr 2017 18:55:44 -0700 Subject: [PATCH 01/13] Wait/retry when cannot delete file on Windows --- mypy/build.py | 4 +-- mypy/test/testutil.py | 42 ++++++++++++++++++++++++++++++ mypy/util.py | 60 ++++++++++++++++++++++++++++++++++++++++++- runtests.py | 1 + typeshed | 2 +- 5 files changed, 105 insertions(+), 4 deletions(-) create mode 100644 mypy/test/testutil.py diff --git a/mypy/build.py b/mypy/build.py index 5ab87ab323f0..2159a6349a3d 100644 --- a/mypy/build.py +++ b/mypy/build.py @@ -901,7 +901,7 @@ def write_cache(id: str, path: str, tree: MypyFile, f.write(data_str) f.write('\n') data_mtime = os.path.getmtime(data_json_tmp) - os.replace(data_json_tmp, data_json) + util.replace(data_json_tmp, data_json) manager.trace("Interface for {} has changed".format(id)) mtime = st.st_mtime @@ -927,7 +927,7 @@ def write_cache(id: str, path: str, tree: MypyFile, json.dump(meta, f, indent=2, sort_keys=True) else: json.dump(meta, f) - os.replace(meta_json_tmp, meta_json) + util.replace(meta_json_tmp, meta_json) return interface_hash diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py new file mode 100644 index 000000000000..b88f8909d68f --- /dev/null +++ b/mypy/test/testutil.py @@ -0,0 +1,42 @@ +import sys +from typing import Type, Callable, List +import time +try: + import collections.abc as collections_abc +except ImportError: + import collections as collections_abc # type: ignore # PY32 and earlier +from unittest import TestCase, main, skipUnless +from mypy import util + + +def create_bad_function(lag: float, exc: BaseException) -> Callable[[], None]: + start_time = time.perf_counter() + + def f() -> None: + if time.perf_counter() - start_time < lag: + raise exc + else: + return + return f + + +def create_funcs() -> List[Callable[[], None]]: + + def linux_function() -> None: pass + windows_function1 = create_bad_function(0.1, PermissionError()) + windows_function2 = create_bad_function(0.2, FileExistsError()) + return [windows_function1, windows_function2, linux_function] + + +class WaitRetryTests(TestCase): + def test_waitfor(self) -> None: + with self.assertRaises(OSError): + util.wait_for(create_funcs(), (PermissionError, FileExistsError), 0.1) + util.wait_for(create_funcs(), (PermissionError, FileExistsError), 1) + util.wait_for(create_funcs(), (OSError,), 1) + with self.assertRaises(FileExistsError): + util.wait_for(create_funcs(), (PermissionError,), 0.4) + + +if __name__ == '__main__': + main() diff --git a/mypy/util.py b/mypy/util.py index 1e8e31898d23..538c7a6308c9 100644 --- a/mypy/util.py +++ b/mypy/util.py @@ -2,8 +2,15 @@ import re import subprocess +import os +import sys +import math +import time +from functools import partial from xml.sax.saxutils import escape -from typing import TypeVar, List, Tuple, Optional, Sequence, Dict +from typing import ( + TypeVar, List, Tuple, Optional, Sequence, Dict, Callable, Iterable, Type, Union, AnyStr, cast +) T = TypeVar('T') @@ -134,3 +141,54 @@ def id(self, o: object) -> int: self.id_map[o] = self.next_id self.next_id += 1 return self.id_map[o] + + +# default timeout is short in case this functions is called individually for many files +# batch processing results in better performance +def wait_for(funcs: Iterable[Callable[[], None]], + exc: Iterable[Type[BaseException]] = (), + timeout: float = 0.1, + msg: str = 'file operations') -> None: + ''' + Execute functions in funcs (without arguments) + Wait and retry all functions that raised exception that matches exc + Increase wait time exponentially, give up after a total wait of timeout seconds + Reraises the latest exception seen if timeout exceeded + ''' + exc = tuple(exc) + last_exc = None + pending = set(funcs) + n_iter = max(1, math.ceil(math.log2(timeout / 0.001))) + for i in range(n_iter): + if not pending: + return + # last wait is ~ timeout/2, so that total wait ~ timeout + wait = timeout / 2 ** (n_iter - i) + failed = set() + for func in pending: + try: + func() + except exc as e: + last_exc = e + failed.add(func) + pending = failed + time.sleep(wait) + sys.stderr.write('timed out waiting for {}'.format(msg)) + raise last_exc + + +if sys.version_info >= (3, 6): + PathType = Union[AnyStr, os.PathLike] +else: + PathType = AnyStr + + +def _replace(src: PathType, dest: PathType) -> None: + repl = cast(Callable[[], None], partial(os.replace, src, dest)) + wait_for([repl], (OSError,), 0.1) + + +if sys.platform.startswith("win"): + replace = _replace +else: + replace = os.replace diff --git a/runtests.py b/runtests.py index 83a6ffa0d3da..1db9f0cd4c47 100755 --- a/runtests.py +++ b/runtests.py @@ -215,6 +215,7 @@ def add_imports(driver: Driver) -> None: 'testdiff', 'testfinegrained', 'testmerge', + 'testutil', ]] diff --git a/typeshed b/typeshed index 8dc082dce5a7..397f99836842 160000 --- a/typeshed +++ b/typeshed @@ -1 +1 @@ -Subproject commit 8dc082dce5a742a0d87270a4494304acec000ec7 +Subproject commit 397f99836842494de2e9561001d16cbe64b3b937 From b089a79adbf25de311c2de5b9cdb3363b37fcbcd Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Tue, 25 Apr 2017 16:00:27 -0700 Subject: [PATCH 02/13] Simplify wait_for; test util.replace directly --- mypy/test/testutil.py | 91 ++++++++++++++++++++++++++++++------------- mypy/util.py | 57 +++++++++------------------ 2 files changed, 83 insertions(+), 65 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index b88f8909d68f..b8b4387da9a8 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -1,41 +1,78 @@ -import sys -from typing import Type, Callable, List +from typing import Iterator import time -try: - import collections.abc as collections_abc -except ImportError: - import collections as collections_abc # type: ignore # PY32 and earlier +import os +import sys +import tempfile +from contextlib import contextmanager +from threading import Thread from unittest import TestCase, main, skipUnless from mypy import util -def create_bad_function(lag: float, exc: BaseException) -> Callable[[], None]: - start_time = time.perf_counter() +WIN32 = sys.platform.startswith("win") + + +@contextmanager +def lock_file(filename: str, duration: float) -> Iterator[Thread]: + ''' + Opens filename (which must exist) for reading + After duration sec, releases the handle + ''' + def _lock_file() -> None: + with open(filename): + time.sleep(duration) + t = Thread(target=_lock_file, daemon=True) + t.start() + yield t + t.join() + + +@skipUnless(WIN32, "only relevant for Windows") +class ReliableReplace(TestCase): + tmpdir = tempfile.TemporaryDirectory(prefix='mypy-test-', + dir=os.path.abspath('tmp-test-dirs')) + src = os.path.join(tmpdir.name, 'tmp1') + dest = os.path.join(tmpdir.name, 'tmp2') + + @classmethod + def tearDownClass(cls) -> None: + cls.tmpdir.cleanup() + + def setUp(self) -> None: + # create two temporary files + for fname in (self.src, self.dest): + with open(fname, 'w') as f: + f.write(fname) + + def replace_ok(self) -> None: + util._replace(self.src, self.dest, timeout=0.25) + self.assertEqual(open(self.dest).read(), self.src, 'replace failed') - def f() -> None: - if time.perf_counter() - start_time < lag: - raise exc - else: - return - return f + def test_normal(self) -> None: + self.replace_ok() + def test_problem_exists(self) -> None: + with lock_file(self.src, 0.1): + with self.assertRaises(PermissionError): + os.replace(self.src, self.dest) -def create_funcs() -> List[Callable[[], None]]: + def test_short_lock_src(self) -> None: + with lock_file(self.src, 0.1): + self.replace_ok() - def linux_function() -> None: pass - windows_function1 = create_bad_function(0.1, PermissionError()) - windows_function2 = create_bad_function(0.2, FileExistsError()) - return [windows_function1, windows_function2, linux_function] + def test_short_lock_dest(self) -> None: + with lock_file(self.dest, 0.1): + self.replace_ok() + def test_long_lock_src(self) -> None: + with lock_file(self.src, 0.4): + with self.assertRaises(PermissionError): + self.replace_ok() -class WaitRetryTests(TestCase): - def test_waitfor(self) -> None: - with self.assertRaises(OSError): - util.wait_for(create_funcs(), (PermissionError, FileExistsError), 0.1) - util.wait_for(create_funcs(), (PermissionError, FileExistsError), 1) - util.wait_for(create_funcs(), (OSError,), 1) - with self.assertRaises(FileExistsError): - util.wait_for(create_funcs(), (PermissionError,), 0.4) + def test_long_lock_dest(self) -> None: + with lock_file(self.dest, 0.4): + with self.assertRaises(PermissionError): + self.replace_ok() if __name__ == '__main__': diff --git a/mypy/util.py b/mypy/util.py index 538c7a6308c9..79ede37cdd9a 100644 --- a/mypy/util.py +++ b/mypy/util.py @@ -6,7 +6,6 @@ import sys import math import time -from functools import partial from xml.sax.saxutils import escape from typing import ( TypeVar, List, Tuple, Optional, Sequence, Dict, Callable, Iterable, Type, Union, AnyStr, cast @@ -143,49 +142,31 @@ def id(self, o: object) -> int: return self.id_map[o] -# default timeout is short in case this functions is called individually for many files -# batch processing results in better performance -def wait_for(funcs: Iterable[Callable[[], None]], - exc: Iterable[Type[BaseException]] = (), - timeout: float = 0.1, - msg: str = 'file operations') -> None: - ''' - Execute functions in funcs (without arguments) - Wait and retry all functions that raised exception that matches exc - Increase wait time exponentially, give up after a total wait of timeout seconds - Reraises the latest exception seen if timeout exceeded - ''' - exc = tuple(exc) - last_exc = None - pending = set(funcs) - n_iter = max(1, math.ceil(math.log2(timeout / 0.001))) - for i in range(n_iter): - if not pending: - return - # last wait is ~ timeout/2, so that total wait ~ timeout - wait = timeout / 2 ** (n_iter - i) - failed = set() - for func in pending: - try: - func() - except exc as e: - last_exc = e - failed.add(func) - pending = failed - time.sleep(wait) - sys.stderr.write('timed out waiting for {}'.format(msg)) - raise last_exc - - if sys.version_info >= (3, 6): PathType = Union[AnyStr, os.PathLike] else: PathType = AnyStr -def _replace(src: PathType, dest: PathType) -> None: - repl = cast(Callable[[], None], partial(os.replace, src, dest)) - wait_for([repl], (OSError,), 0.1) +def _replace(src: PathType, dest: PathType, timeout: float = 10) -> None: + ''' + Replace src with dest using os.replace(src, dest) + Wait and retry if OSError exception is raised + Increase wait time exponentially until total wait of timeout sec + On timeout, give up and reraise the last exception seen + ''' + n_iter = max(1, math.ceil(math.log2(timeout / 0.001))) + for i in range(n_iter): + # last wait is ~ timeout/2, so that total wait ~ timeout + wait = timeout / 2 ** (n_iter - i - 1) + try: + os.replace(src, dest) + except PermissionError: + if i == n_iter - 1: + raise + else: + return + time.sleep(wait) if sys.platform.startswith("win"): From 42c6987bbf637ccfda713a52ef6fe9a07d98350b Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Tue, 25 Apr 2017 17:32:30 -0700 Subject: [PATCH 03/13] Refactor tests to create a separate src/dest each time --- mypy/test/testutil.py | 64 ++++++++++++++++++++++--------------------- 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index b8b4387da9a8..d2d7b600d762 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -1,4 +1,4 @@ -from typing import Iterator +from typing import Iterator, Tuple import time import os import sys @@ -7,6 +7,7 @@ from threading import Thread from unittest import TestCase, main, skipUnless from mypy import util +from mypy.build import random_string WIN32 = sys.platform.startswith("win") @@ -29,50 +30,51 @@ def _lock_file() -> None: @skipUnless(WIN32, "only relevant for Windows") class ReliableReplace(TestCase): + # will be cleaned up automatically when this class goes out of scope tmpdir = tempfile.TemporaryDirectory(prefix='mypy-test-', dir=os.path.abspath('tmp-test-dirs')) - src = os.path.join(tmpdir.name, 'tmp1') - dest = os.path.join(tmpdir.name, 'tmp2') + timeout = 0.5 + short_lock = 0.2 + long_lock = 1 - @classmethod - def tearDownClass(cls) -> None: - cls.tmpdir.cleanup() - - def setUp(self) -> None: + @contextmanager + def prepare_src_dest(self, src_lock_duration: float, dest_lock_duration: float + ) -> Iterator[Tuple[str, str]]: # create two temporary files - for fname in (self.src, self.dest): + src = os.path.join(self.tmpdir.name, random_string()) + dest = os.path.join(self.tmpdir.name, random_string()) + + for fname in (src, dest): with open(fname, 'w') as f: f.write(fname) - def replace_ok(self) -> None: - util._replace(self.src, self.dest, timeout=0.25) - self.assertEqual(open(self.dest).read(), self.src, 'replace failed') + with lock_file(src, src_lock_duration): + with lock_file(dest, dest_lock_duration): + yield src, dest + + def replace_ok(self, src_lock_duration: float, dest_lock_duration: float, + timeout: float) -> None: + with self.prepare_src_dest(src_lock_duration, dest_lock_duration) as (src, dest): + util._replace(src, dest, timeout=timeout) + self.assertEqual(open(dest).read(), src, 'replace failed') def test_normal(self) -> None: - self.replace_ok() + self.replace_ok(0, 0, self.timeout) def test_problem_exists(self) -> None: - with lock_file(self.src, 0.1): + with self.prepare_src_dest(self.short_lock, 0) as (src, dest): with self.assertRaises(PermissionError): - os.replace(self.src, self.dest) - - def test_short_lock_src(self) -> None: - with lock_file(self.src, 0.1): - self.replace_ok() + os.replace(src, dest) - def test_short_lock_dest(self) -> None: - with lock_file(self.dest, 0.1): - self.replace_ok() + def test_short_lock(self) -> None: + self.replace_ok(self.short_lock, 0, self.timeout) + self.replace_ok(0, self.short_lock, self.timeout) - def test_long_lock_src(self) -> None: - with lock_file(self.src, 0.4): - with self.assertRaises(PermissionError): - self.replace_ok() - - def test_long_lock_dest(self) -> None: - with lock_file(self.dest, 0.4): - with self.assertRaises(PermissionError): - self.replace_ok() + def test_long_lock(self) -> None: + with self.assertRaises(PermissionError): + self.replace_ok(self.long_lock, 0, self.timeout) + with self.assertRaises(PermissionError): + self.replace_ok(0, self.long_lock, self.timeout) if __name__ == '__main__': From 2d369c0955294c4d515141a691d7d66076340031 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Tue, 25 Apr 2017 17:51:57 -0700 Subject: [PATCH 04/13] Wait for lock expiration at the end --- mypy/test/testutil.py | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index d2d7b600d762..92e397de723b 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -1,4 +1,4 @@ -from typing import Iterator, Tuple +from typing import Iterator, List, Tuple import time import os import sys @@ -13,8 +13,7 @@ WIN32 = sys.platform.startswith("win") -@contextmanager -def lock_file(filename: str, duration: float) -> Iterator[Thread]: +def lock_file(filename: str, duration: float) -> Thread: ''' Opens filename (which must exist) for reading After duration sec, releases the handle @@ -24,8 +23,7 @@ def _lock_file() -> None: time.sleep(duration) t = Thread(target=_lock_file, daemon=True) t.start() - yield t - t.join() + return t @skipUnless(WIN32, "only relevant for Windows") @@ -33,13 +31,19 @@ class ReliableReplace(TestCase): # will be cleaned up automatically when this class goes out of scope tmpdir = tempfile.TemporaryDirectory(prefix='mypy-test-', dir=os.path.abspath('tmp-test-dirs')) - timeout = 0.5 - short_lock = 0.2 - long_lock = 1 + timeout = 1 + short_lock = 0.25 + long_lock = 2 + + threads = [] # type: List[Thread] + + @classmethod + def tearDownClass(cls): + for t in cls.threads: + t.join() - @contextmanager def prepare_src_dest(self, src_lock_duration: float, dest_lock_duration: float - ) -> Iterator[Tuple[str, str]]: + ) -> Tuple[str, str]: # create two temporary files src = os.path.join(self.tmpdir.name, random_string()) dest = os.path.join(self.tmpdir.name, random_string()) @@ -48,23 +52,23 @@ def prepare_src_dest(self, src_lock_duration: float, dest_lock_duration: float with open(fname, 'w') as f: f.write(fname) - with lock_file(src, src_lock_duration): - with lock_file(dest, dest_lock_duration): - yield src, dest + self.threads.append(lock_file(src, src_lock_duration)) + self.threads.append(lock_file(dest, dest_lock_duration)) + return src, dest def replace_ok(self, src_lock_duration: float, dest_lock_duration: float, timeout: float) -> None: - with self.prepare_src_dest(src_lock_duration, dest_lock_duration) as (src, dest): - util._replace(src, dest, timeout=timeout) - self.assertEqual(open(dest).read(), src, 'replace failed') + src, dest = self.prepare_src_dest(src_lock_duration, dest_lock_duration) + util._replace(src, dest, timeout=timeout) + self.assertEqual(open(dest).read(), src, 'replace failed') def test_normal(self) -> None: self.replace_ok(0, 0, self.timeout) def test_problem_exists(self) -> None: - with self.prepare_src_dest(self.short_lock, 0) as (src, dest): - with self.assertRaises(PermissionError): - os.replace(src, dest) + src, dest = self.prepare_src_dest(self.short_lock, 0) + with self.assertRaises(PermissionError): + os.replace(src, dest) def test_short_lock(self) -> None: self.replace_ok(self.short_lock, 0, self.timeout) From 8e61b79f396afc8a8af79e8e46a53dc4c75c88e5 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Tue, 25 Apr 2017 18:08:12 -0700 Subject: [PATCH 05/13] Move all tests into one because xdist xdist isn't flexible enough to run a given set of tests in one process If these tests are split into multiple processes, they will take a lot longer since each will wait for locks to die out at the end. --- mypy/test/testutil.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index 92e397de723b..8ac79876e5fc 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -38,7 +38,7 @@ class ReliableReplace(TestCase): threads = [] # type: List[Thread] @classmethod - def tearDownClass(cls): + def tearDownClass(cls) -> None: for t in cls.threads: t.join() @@ -62,19 +62,17 @@ def replace_ok(self, src_lock_duration: float, dest_lock_duration: float, util._replace(src, dest, timeout=timeout) self.assertEqual(open(dest).read(), src, 'replace failed') - def test_normal(self) -> None: + def test_everything(self) -> None: + # normal use, no locks self.replace_ok(0, 0, self.timeout) - - def test_problem_exists(self) -> None: + # make sure the problem is real src, dest = self.prepare_src_dest(self.short_lock, 0) with self.assertRaises(PermissionError): os.replace(src, dest) - - def test_short_lock(self) -> None: + # short lock self.replace_ok(self.short_lock, 0, self.timeout) self.replace_ok(0, self.short_lock, self.timeout) - - def test_long_lock(self) -> None: + # long lock with self.assertRaises(PermissionError): self.replace_ok(self.long_lock, 0, self.timeout) with self.assertRaises(PermissionError): From beb453fd5733bd231a24d654337fc3e88420746b Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Wed, 26 Apr 2017 12:55:34 -0700 Subject: [PATCH 06/13] Address CR --- mypy/test/testutil.py | 19 +++++++++---------- mypy/util.py | 23 +++++++++++++---------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index 8ac79876e5fc..534b74ca3956 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -14,10 +14,7 @@ def lock_file(filename: str, duration: float) -> Thread: - ''' - Opens filename (which must exist) for reading - After duration sec, releases the handle - ''' + """Open a file and keep it open in a background thread for duration sec.""" def _lock_file() -> None: with open(filename): time.sleep(duration) @@ -28,7 +25,6 @@ def _lock_file() -> None: @skipUnless(WIN32, "only relevant for Windows") class ReliableReplace(TestCase): - # will be cleaned up automatically when this class goes out of scope tmpdir = tempfile.TemporaryDirectory(prefix='mypy-test-', dir=os.path.abspath('tmp-test-dirs')) timeout = 1 @@ -39,12 +35,15 @@ class ReliableReplace(TestCase): @classmethod def tearDownClass(cls) -> None: + # Need to wait for threads to complete, otherwise we'll get PermissionError + # at the end (whether tmpdir goes out of scope or we explicitly call cleanup). for t in cls.threads: t.join() + cls.tmpdir.cleanup() def prepare_src_dest(self, src_lock_duration: float, dest_lock_duration: float ) -> Tuple[str, str]: - # create two temporary files + # Create two temporary files with random names. src = os.path.join(self.tmpdir.name, random_string()) dest = os.path.join(self.tmpdir.name, random_string()) @@ -63,16 +62,16 @@ def replace_ok(self, src_lock_duration: float, dest_lock_duration: float, self.assertEqual(open(dest).read(), src, 'replace failed') def test_everything(self) -> None: - # normal use, no locks + # No files locked. self.replace_ok(0, 0, self.timeout) - # make sure the problem is real + # Make sure we can reproduce the issue with our setup. src, dest = self.prepare_src_dest(self.short_lock, 0) with self.assertRaises(PermissionError): os.replace(src, dest) - # short lock + # Lock files for a time short enough that util.replace won't timeout. self.replace_ok(self.short_lock, 0, self.timeout) self.replace_ok(0, self.short_lock, self.timeout) - # long lock + # Lock files for a time long enough that util.replace times out. with self.assertRaises(PermissionError): self.replace_ok(self.long_lock, 0, self.timeout) with self.assertRaises(PermissionError): diff --git a/mypy/util.py b/mypy/util.py index 79ede37cdd9a..e6ff23f10910 100644 --- a/mypy/util.py +++ b/mypy/util.py @@ -143,21 +143,24 @@ def id(self, o: object) -> int: if sys.version_info >= (3, 6): - PathType = Union[AnyStr, os.PathLike] + PathType = Union[AnyStr, 'os.PathLike[AnyStr]'] else: - PathType = AnyStr + # Workaround to allow the use of generic in _replace + class _PathType(Generic[AnyStr]): ... + PathType = _PathType[AnyStr] -def _replace(src: PathType, dest: PathType, timeout: float = 10) -> None: - ''' - Replace src with dest using os.replace(src, dest) - Wait and retry if OSError exception is raised - Increase wait time exponentially until total wait of timeout sec - On timeout, give up and reraise the last exception seen - ''' +def _replace(src: PathType[AnyStr], dest: PathType[AnyStr], timeout: float = 1) -> None: + """Replace src with dest using os.replace(src, dest). + + Wait and retry if OSError exception is raised. + + Increase wait time exponentially until total wait of timeout sec; + on timeout, give up and reraise the last exception seen. + """ n_iter = max(1, math.ceil(math.log2(timeout / 0.001))) for i in range(n_iter): - # last wait is ~ timeout/2, so that total wait ~ timeout + # Last wait is ~ timeout/2, so that total wait ~ timeout. wait = timeout / 2 ** (n_iter - i - 1) try: os.replace(src, dest) From 2f3e380613dc1a3efd3d475d8f41e19d0455b35c Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Wed, 26 Apr 2017 14:16:10 -0700 Subject: [PATCH 07/13] No math :) --- mypy/util.py | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/mypy/util.py b/mypy/util.py index e6ff23f10910..a0cce1b7cda1 100644 --- a/mypy/util.py +++ b/mypy/util.py @@ -6,9 +6,11 @@ import sys import math import time +from itertools import count from xml.sax.saxutils import escape from typing import ( - TypeVar, List, Tuple, Optional, Sequence, Dict, Callable, Iterable, Type, Union, AnyStr, cast + TypeVar, List, Tuple, Optional, Sequence, Dict, Callable, Iterable, Type, Union, + AnyStr, Generic ) @@ -158,18 +160,27 @@ def _replace(src: PathType[AnyStr], dest: PathType[AnyStr], timeout: float = 1) Increase wait time exponentially until total wait of timeout sec; on timeout, give up and reraise the last exception seen. """ - n_iter = max(1, math.ceil(math.log2(timeout / 0.001))) - for i in range(n_iter): - # Last wait is ~ timeout/2, so that total wait ~ timeout. - wait = timeout / 2 ** (n_iter - i - 1) + # Find starting value for wait. + wait = timeout + total_wait = 0 # type: float + while True: + if wait < 0.001: + break + wait /= 2 + while True: try: os.replace(src, dest) except PermissionError: - if i == n_iter - 1: + # Can't compare for equality because float arithmetic; + # if we hit 0.9999 * timeout, it's time to stop. + # For timeout more than a few ms, we'll be very close to the desired timeout. + if total_wait > 0.99 * timeout: raise else: return time.sleep(wait) + total_wait += wait + wait *= 2 if sys.platform.startswith("win"): From 4950918a72bfee99c650bd85951434686135ac0f Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Wed, 26 Apr 2017 16:10:27 -0700 Subject: [PATCH 08/13] Simplify util.replace, no need to enforce precise timeout --- mypy/test/testutil.py | 9 +++++---- mypy/util.py | 15 +++------------ 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index 534b74ca3956..aecb1290ee10 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -24,12 +24,13 @@ def _lock_file() -> None: @skipUnless(WIN32, "only relevant for Windows") -class ReliableReplace(TestCase): +class WindowsReplace(TestCase): tmpdir = tempfile.TemporaryDirectory(prefix='mypy-test-', dir=os.path.abspath('tmp-test-dirs')) - timeout = 1 - short_lock = 0.25 - long_lock = 2 + # Choose timeout value that would ensure actual wait inside util.replace is close to timeout + timeout = 0.0009 * 2 ** 10 + short_lock = timeout / 4 + long_lock = timeout * 2 threads = [] # type: List[Thread] diff --git a/mypy/util.py b/mypy/util.py index a0cce1b7cda1..f72d1fb3148c 100644 --- a/mypy/util.py +++ b/mypy/util.py @@ -160,26 +160,17 @@ def _replace(src: PathType[AnyStr], dest: PathType[AnyStr], timeout: float = 1) Increase wait time exponentially until total wait of timeout sec; on timeout, give up and reraise the last exception seen. """ - # Find starting value for wait. - wait = timeout - total_wait = 0 # type: float - while True: - if wait < 0.001: - break - wait /= 2 + wait = 0.001 while True: try: os.replace(src, dest) except PermissionError: - # Can't compare for equality because float arithmetic; - # if we hit 0.9999 * timeout, it's time to stop. - # For timeout more than a few ms, we'll be very close to the desired timeout. - if total_wait > 0.99 * timeout: + # The actual total wait might be up to 2x larger than timeout parameter + if wait > timeout: raise else: return time.sleep(wait) - total_wait += wait wait *= 2 From 0cc63375054735a6cd6be409b79c591c113d68a0 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Fri, 28 Apr 2017 16:56:06 -0700 Subject: [PATCH 09/13] Address CR --- mypy/test/testutil.py | 12 +++++++++++- mypy/util.py | 5 +---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index aecb1290ee10..d648ccded323 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -44,7 +44,11 @@ def tearDownClass(cls) -> None: def prepare_src_dest(self, src_lock_duration: float, dest_lock_duration: float ) -> Tuple[str, str]: - # Create two temporary files with random names. + '''Create two files in self.tmpdir random names (src, dest) and unique contents; + then spawn two threads that lock each of them for a specified duration. + + Return a tuple (src, dest). + ''' src = os.path.join(self.tmpdir.name, random_string()) dest = os.path.join(self.tmpdir.name, random_string()) @@ -58,6 +62,12 @@ def prepare_src_dest(self, src_lock_duration: float, dest_lock_duration: float def replace_ok(self, src_lock_duration: float, dest_lock_duration: float, timeout: float) -> None: + '''Check whether util._replace, called with a specified timeout, + worked successfully on two newly created files locked for specified + durations. + + Return True if the replacement succeeded. + ''' src, dest = self.prepare_src_dest(src_lock_duration, dest_lock_duration) util._replace(src, dest, timeout=timeout) self.assertEqual(open(dest).read(), src, 'replace failed') diff --git a/mypy/util.py b/mypy/util.py index f72d1fb3148c..773e9552d5bd 100644 --- a/mypy/util.py +++ b/mypy/util.py @@ -4,9 +4,7 @@ import subprocess import os import sys -import math import time -from itertools import count from xml.sax.saxutils import escape from typing import ( TypeVar, List, Tuple, Optional, Sequence, Dict, Callable, Iterable, Type, Union, @@ -164,12 +162,11 @@ def _replace(src: PathType[AnyStr], dest: PathType[AnyStr], timeout: float = 1) while True: try: os.replace(src, dest) + return except PermissionError: # The actual total wait might be up to 2x larger than timeout parameter if wait > timeout: raise - else: - return time.sleep(wait) wait *= 2 From f05647ce41f328750b7a0eaa18bacbd0d64864a5 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Fri, 28 Apr 2017 18:18:33 -0700 Subject: [PATCH 10/13] Fix quotes --- mypy/test/testutil.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index d648ccded323..e64ed58f52e3 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -44,11 +44,11 @@ def tearDownClass(cls) -> None: def prepare_src_dest(self, src_lock_duration: float, dest_lock_duration: float ) -> Tuple[str, str]: - '''Create two files in self.tmpdir random names (src, dest) and unique contents; + """Create two files in self.tmpdir random names (src, dest) and unique contents; then spawn two threads that lock each of them for a specified duration. Return a tuple (src, dest). - ''' + """ src = os.path.join(self.tmpdir.name, random_string()) dest = os.path.join(self.tmpdir.name, random_string()) @@ -62,12 +62,12 @@ def prepare_src_dest(self, src_lock_duration: float, dest_lock_duration: float def replace_ok(self, src_lock_duration: float, dest_lock_duration: float, timeout: float) -> None: - '''Check whether util._replace, called with a specified timeout, + """Check whether util._replace, called with a specified timeout, worked successfully on two newly created files locked for specified durations. Return True if the replacement succeeded. - ''' + """ src, dest = self.prepare_src_dest(src_lock_duration, dest_lock_duration) util._replace(src, dest, timeout=timeout) self.assertEqual(open(dest).read(), src, 'replace failed') From f9de4c9a699cdd8597d51906d5cf77475115d246 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Fri, 28 Apr 2017 22:47:40 -0700 Subject: [PATCH 11/13] Split tests according to their purpose --- mypy/test/testutil.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index e64ed58f52e3..00ac7770f2cd 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -72,16 +72,22 @@ def replace_ok(self, src_lock_duration: float, dest_lock_duration: float, util._replace(src, dest, timeout=timeout) self.assertEqual(open(dest).read(), src, 'replace failed') - def test_everything(self) -> None: + def test_no_locks(self) -> None: # No files locked. self.replace_ok(0, 0, self.timeout) + + def test_original_problem(self) -> None: # Make sure we can reproduce the issue with our setup. src, dest = self.prepare_src_dest(self.short_lock, 0) with self.assertRaises(PermissionError): os.replace(src, dest) + + def test_short_locks(self) -> None: # Lock files for a time short enough that util.replace won't timeout. self.replace_ok(self.short_lock, 0, self.timeout) self.replace_ok(0, self.short_lock, self.timeout) + + def test_long_locks(self) -> None: # Lock files for a time long enough that util.replace times out. with self.assertRaises(PermissionError): self.replace_ok(self.long_lock, 0, self.timeout) From 199a103229a848a7aead9e86b7866ade37fc5ed0 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Fri, 28 Apr 2017 23:37:10 -0700 Subject: [PATCH 12/13] Prevent race condition --- mypy/test/testutil.py | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index 00ac7770f2cd..9dbdb32e55ea 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -1,4 +1,4 @@ -from typing import Iterator, List, Tuple +from typing import Iterator, List, Tuple, IO import time import os import sys @@ -13,16 +13,6 @@ WIN32 = sys.platform.startswith("win") -def lock_file(filename: str, duration: float) -> Thread: - """Open a file and keep it open in a background thread for duration sec.""" - def _lock_file() -> None: - with open(filename): - time.sleep(duration) - t = Thread(target=_lock_file, daemon=True) - t.start() - return t - - @skipUnless(WIN32, "only relevant for Windows") class WindowsReplace(TestCase): tmpdir = tempfile.TemporaryDirectory(prefix='mypy-test-', @@ -34,6 +24,18 @@ class WindowsReplace(TestCase): threads = [] # type: List[Thread] + @classmethod + def close_file_after(cls, file: IO, delay: float) -> Thread: + """Start a background thread to close file after delay sec.""" + def _close_file_after() -> None: + time.sleep(delay) + file.close() + + t = Thread(target=_close_file_after, daemon=True) + cls.threads.append(t) + t.start() + return t + @classmethod def tearDownClass(cls) -> None: # Need to wait for threads to complete, otherwise we'll get PermissionError @@ -52,12 +54,14 @@ def prepare_src_dest(self, src_lock_duration: float, dest_lock_duration: float src = os.path.join(self.tmpdir.name, random_string()) dest = os.path.join(self.tmpdir.name, random_string()) - for fname in (src, dest): - with open(fname, 'w') as f: - f.write(fname) + for fname, delay in zip((src, dest), (src_lock_duration, dest_lock_duration)): + f = open(fname, 'w') + f.write(fname) + if delay: + self.close_file_after(f, delay) + else: + f.close() - self.threads.append(lock_file(src, src_lock_duration)) - self.threads.append(lock_file(dest, dest_lock_duration)) return src, dest def replace_ok(self, src_lock_duration: float, dest_lock_duration: float, From a21f6b585f480d516bd6849bae0399df35f02423 Mon Sep 17 00:00:00 2001 From: Max Moroz Date: Sun, 30 Apr 2017 00:05:09 -0700 Subject: [PATCH 13/13] CR fixes --- mypy/test/testutil.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/mypy/test/testutil.py b/mypy/test/testutil.py index 9dbdb32e55ea..db7c410591c9 100644 --- a/mypy/test/testutil.py +++ b/mypy/test/testutil.py @@ -74,14 +74,16 @@ def replace_ok(self, src_lock_duration: float, dest_lock_duration: float, """ src, dest = self.prepare_src_dest(src_lock_duration, dest_lock_duration) util._replace(src, dest, timeout=timeout) - self.assertEqual(open(dest).read(), src, 'replace failed') + # Note that dest handle may still be open but reading from it is ok. + with open(dest) as f: + self.assertEqual(f.read(), src, 'replace failed') def test_no_locks(self) -> None: # No files locked. self.replace_ok(0, 0, self.timeout) def test_original_problem(self) -> None: - # Make sure we can reproduce the issue with our setup. + # Make sure we can reproduce https://github.com/python/mypy/issues/3215 with our setup. src, dest = self.prepare_src_dest(self.short_lock, 0) with self.assertRaises(PermissionError): os.replace(src, dest)