Repository: glotzerlab/signac
Pull request #850 (2022-11-03 15:53:56)

Reenable multithreading tests
<!-- Provide a general summary of your changes in the title above. -->

## Description
<!-- Describe your changes in detail. -->
<!-- Please indicate if the changes may break existing functionality. -->
Tests of multithreaded support for synced collections were added as part of the development effort. The tests were generally reliable, but we saw sporadic failures on Windows for unknown reasons, so they were marked as conditionally expected to fail on Windows (but done so incorrectly, see below). The tests were changed to be unconditionally `xfail`ed due to development difficulties in #777. This PR adds them back in with a more target scope for skipping. This change also allows us to enable `xfail_strict` in our testing.

## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
<!-- If it fixes an open issue, please link to the issue here. -->
I finally have access to a Windows machine myself, so I looked into these again this morning. I cannot reproduce the failures locally on WSL no matter what I try (with/without pytest-xdist, many repetitions with pytest-repeat, etc), but I _can_ reproduce the failures in normal Windows. The error is consistently a `PermissionError` encountered in the `os.replace` call. There is a [Python bug clearly indicating that Python intentionally avoids documenting such incompatibilities, but that this is a known problem with `os.replace` in multithreaded settings on Windows due to the underlying filesystem instruction](https://bugs.python.org/issue46003). As such, we should not expect this to work on Windows, and the original exclusion Windows-based `skip` prior to #777 is correct, not the current blanket `xfail`.

[Our previous thread justifying the change in testing behavior](https://github.com/glotzerlab/signac/pull/777#discussion_r901175366) identified that there was a bug in our old filtering. We were using `"win" in platform.python_implementation()` as a way to identify whether we were on Windows, and using that to determine whether or not to run the tests. `platform.python_implementation()` indicates what _Python_ implementation we are using (CPython, Jython, PyPy, etc), not the operating system, so this filter was clearly incorrect. However, the other claim in that thread is also obviously false:
> Apparently this was getting WINDOWS = True because of that bug, and hiding failures on other platforms too:

At the time we ran tests on Pypy and CPython, and both `"win" in "CPython"` and `"win" in PyPy` are always `False`, not always `True`. The effect of this error therefore could not have been to hide failures on non-Windows platforms, all it could have done is run tests on Windows that we expected to fail. Therefore, my suspicion is that what was observed in that PR were only Windows-specific errors. There were additional reports of failures in #776, but they appear to have been scattered among various other failures that occurred during the CI transition, so it is unclear whether we ever had reliable examples of failures on other operating systems.

This PR reenables these tests on the platforms where we should expect them to succeed. Given the upcoming release of signac 2.0, if we start seeing failures in these tests again we should reinvestigate our claims of multithreading support. The tests don't affect any behavior that signac relies on, only otherwise unused features of synced collections, so this is mostly relevant for documenting synced collections when we split those into a separate package post 2.0. 

## Checklist:
<!-- This checklist must be complete before merging the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [x] I am familiar with the [Contributing Guidelines](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [Contributor Agreement](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.
- [ ] The [package documentation](https://github.com/glotzerlab/signac/tree/master/doc) and [framework documentation](https://docs.signac.io/) in [signac-docs](https://github.com/glotzerlab/signac-docs) are up to date with these changes.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added any related issue and pull request numbers for future reference.


vyasr: I'm happy to rerun tests on this PR a few times to see if we can reproduce the failures again before we merge.
codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/850?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) Report
> Merging [#850](https://codecov.io/gh/glotzerlab/signac/pull/850?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (eefc61c) into [master](https://codecov.io/gh/glotzerlab/signac/commit/f5574f9ccb6522a94cc80ae58db912267d30325d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (f5574f9) will **decrease** coverage by `0.23%`.
> The diff coverage is `71.42%`.

```diff
@@            Coverage Diff             @@
##           master     #850      +/-   ##
==========================================
- Coverage   85.62%   85.38%   -0.24%     
==========================================
  Files          51       51              
  Lines        4641     4647       +6     
  Branches      917      918       +1     
==========================================
- Hits         3974     3968       -6     
- Misses        481      488       +7     
- Partials      186      191       +5     
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/850?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) | Coverage Δ | |
|---|---|---|
| [...ac/\_synced\_collections/backends/collection\_json.py](https://codecov.io/gh/glotzerlab/signac/pull/850/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYmFja2VuZHMvY29sbGVjdGlvbl9qc29uLnB5) | `96.82% <71.42%> (-3.18%)` | :arrow_down: |
| [signac/core/utility.py](https://codecov.io/gh/glotzerlab/signac/pull/850/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvcmUvdXRpbGl0eS5weQ==) | `71.42% <0.00%> (-28.58%)` | :arrow_down: |
| [...ed\_collections/buffers/file\_buffered\_collection.py](https://codecov.io/gh/glotzerlab/signac/pull/850/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19zeW5jZWRfY29sbGVjdGlvbnMvYnVmZmVycy9maWxlX2J1ZmZlcmVkX2NvbGxlY3Rpb24ucHk=) | `95.76% <0.00%> (-1.70%)` | :arrow_down: |
| [signac/\_\_main\_\_.py](https://codecov.io/gh/glotzerlab/signac/pull/850/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL19fbWFpbl9fLnB5) | `77.40% <0.00%> (-0.61%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab)

bdice: @csadorf @vyasr I'll let you two sort out the rest of this PR and merge it. I'm going to focus on a few other items for 2.0.
bdice: @vyasr Excluding both `win32` and `cygwin` was correct. I tested it locally and reverted my previous changes. I will automerge this PR.

Patch:
diff --git a/changelog.txt b/changelog.txt
index 3733d03d..974bb730 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -30,6 +30,7 @@ Changed
  - Rather than searching upwards until the root, ``load_config`` will only load configuration files in the specified directory, which is assumed to be a project directory, as well as the user's home directory (#711).
  - Changed the ``root`` parameter to ``path`` in the ``signac.get_project`` and ``signac.init_project`` functions and corresponding ``Project`` methods (#757, #758).
  - The prefix argument to ``$ signac view`` is now optional and can be provided with ``-p/--prefix`` (#653, #774).
+ - Tests are run with ``xfail_strict = True`` (#850).
  - Detection of an invalid config will raise an error rather than a debug log (#855).
 
 Removed
diff --git a/setup.cfg b/setup.cfg
index 9d7a4113..5032af20 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -35,6 +35,7 @@ omit =
 	*/signac/common/deprecation/*.py
 
 [tool:pytest]
+xfail_strict = True
 filterwarnings = 
 	error::FutureWarning:signac.*
 	error::DeprecationWarning:signac.*
diff --git a/signac/_synced_collections/backends/collection_json.py b/signac/_synced_collections/backends/collection_json.py
index 8611908a..df7eece8 100644
--- a/signac/_synced_collections/backends/collection_json.py
+++ b/signac/_synced_collections/backends/collection_json.py
@@ -6,6 +6,7 @@
 import errno
 import json
 import os
+import sys
 import uuid
 from collections.abc import Mapping, Sequence
 from typing import Callable, FrozenSet
@@ -27,6 +28,9 @@ from ..numpy_utils import (
 from ..utils import AbstractTypeResolver, SyncedCollectionJSONEncoder
 from ..validators import json_format_validator, no_dot_in_key, require_string_key
 
+ON_WINDOWS = sys.platform.startswith("win32") or sys.platform.startswith("cygwin")
+
+
 """
 There are many classes defined in this file. Most of the definitions are
 trivial since logic is largely inherited, but the large number of classes
@@ -127,11 +131,13 @@ class JSONCollection(SyncedCollection):
 
     **Thread safety**
 
-    The :class:`JSONCollection` is thread-safe. To make these collections safe, the
-    ``write_concern`` flag is ignored in multithreaded execution, and the
-    write is **always** performed via a write to temporary file followed by a
+    The :class:`JSONCollection` is thread-safe on Unix-like systems (not
+    Windows, see the Warnings section). To make these collections safe, the
+    ``write_concern`` flag is ignored in multithreaded execution, and the write
+    is **always** performed via a write to temporary file followed by a
     replacement of the original file. The file replacement operation uses
-    :func:`os.replace`, which is guaranteed to be atomic by the Python standard.
+    :func:`os.replace`, which is guaranteed to be atomic by the Python
+    standard.
 
     Parameters
     ----------
@@ -145,10 +151,17 @@ class JSONCollection(SyncedCollection):
     \*\*kwargs :
         Keyword arguments forwarded to parent constructors.
 
+    Warnings
+    --------
+    This class is _not_ thread safe on Windows. It relies on ``os.replace`` for
+    atomic file writes, and that method can fail in multithreaded situations if
+    open handles exist to the destination file within the same process on a
+    different thread. See https://bugs.python.org/issue46003 for more
+    information.
     """
 
     _backend = __name__  # type: ignore
-    _supports_threading = True
+    _supports_threading = not ON_WINDOWS
     _validators: Sequence_t[Callable] = (require_string_key, json_format_validator)
 
     def __init__(self, filename=None, write_concern=False, *args, **kwargs):
@@ -200,9 +213,21 @@ class JSONCollection(SyncedCollection):
 
     @property
     def filename(self):
-        """str: The name of the associated JSON file on disk."""
+        """str: Get or set the name of the associated JSON file on disk."""
         return self._filename
 
+    @filename.setter
+    def filename(self, value):
+        # When setting the filename we must also remap the locks.
+        with self._thread_lock:
+            if type(self)._threading_support_is_active:
+                old_lock_id = self._lock_id
+
+            self._filename = value
+
+            if type(self)._threading_support_is_active:
+                type(self)._locks[self._lock_id] = type(self)._locks.pop(old_lock_id)
+
     @property
     def _lock_id(self):
         return self._filename
@@ -227,6 +252,7 @@ _JSONDICT_PROTECTED_KEYS = frozenset(
         "registry",
         # These keys are specific to the JSON backend.
         "_filename",
+        "filename",
         "_write_concern",
     )
 )
diff --git a/signac/contrib/job.py b/signac/contrib/job.py
index 909d8b69..d02df786 100644
--- a/signac/contrib/job.py
+++ b/signac/contrib/job.py
@@ -123,18 +123,15 @@ class _StatePointDict(JSONAttrDict):
             job._initialize_lazy_properties()
 
         # Remove the temporary state point file if it was created. Have to do it
-        # here because we need to get the updated job state point filename.
+        # here so that we can get the updated job state point filename and set
+        # it to self.filename below.
         try:
             os.remove(job._statepoint_filename + "~")
         except OSError as error:
             if error.errno != errno.ENOENT:
                 raise
 
-        # Since all the jobs are equivalent, just grab the filename from the
-        # last one and init it. Also migrate the lock for multithreaded support.
-        old_lock_id = self._lock_id
-        self._filename = job._statepoint_filename
-        type(self)._locks[self._lock_id] = type(self)._locks.pop(old_lock_id)
+        self.filename = job._statepoint_filename
 
         if should_init:
             # Only initializing one job assumes that all changes in init are
@@ -164,7 +161,7 @@ class _StatePointDict(JSONAttrDict):
         """
         try:
             # Open the file for writing only if it does not exist yet.
-            if force or not os.path.isfile(self._filename):
+            if force or not os.path.isfile(self.filename):
                 super()._save()
         except Exception as error:
             if not isinstance(error, OSError) or error.errno not in (
@@ -174,7 +171,7 @@ class _StatePointDict(JSONAttrDict):
                 # Attempt to delete the file on error, to prevent corruption.
                 # OSErrors that are EEXIST or EACCES don't need to delete the file.
                 try:
-                    os.remove(self._filename)
+                    os.remove(self.filename)
                 except Exception:  # ignore all errors here
                     pass
                 raise


Test patch:
diff --git a/tests/test_synced_collections/synced_collection_test.py b/tests/test_synced_collections/synced_collection_test.py
index b5c9ce58..87064892 100644
--- a/tests/test_synced_collections/synced_collection_test.py
+++ b/tests/test_synced_collections/synced_collection_test.py
@@ -451,13 +451,6 @@ class SyncedDictTest(SyncedCollectionTest):
             with pytest.raises(TypeError):
                 synced_collection[key] = testdata
 
-    @pytest.mark.xfail(
-        reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
-        ),
-    )
     def test_multithreaded(self, synced_collection):
         """Test multithreaded runs of synced dicts."""
         if not type(synced_collection)._supports_threading:
@@ -770,13 +763,6 @@ class SyncedListTest(SyncedCollectionTest):
         assert isinstance(child2, SyncedCollection)
         assert isinstance(child1, SyncedCollection)
 
-    @pytest.mark.xfail(
-        reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
-        ),
-    )
     def test_multithreaded(self, synced_collection):
         """Test multithreaded runs of synced lists."""
         if not type(synced_collection)._supports_threading:
diff --git a/tests/test_synced_collections/test_json_buffered_collection.py b/tests/test_synced_collections/test_json_buffered_collection.py
index a5677698..6302bbf9 100644
--- a/tests/test_synced_collections/test_json_buffered_collection.py
+++ b/tests/test_synced_collections/test_json_buffered_collection.py
@@ -4,6 +4,7 @@
 import itertools
 import json
 import os
+import sys
 import time
 from concurrent.futures import ThreadPoolExecutor
 
@@ -23,6 +24,8 @@ from signac._synced_collections.backends.collection_json import (
 )
 from signac._synced_collections.errors import BufferedError, MetadataError
 
+ON_WINDOWS = sys.platform.startswith("win32") or sys.platform.startswith("cygwin")
+
 
 class BufferedJSONCollectionTest(JSONCollectionTest):
     def load(self, collection):
@@ -376,11 +379,11 @@ class TestBufferedJSONDict(BufferedJSONCollectionTest, TestJSONDict):
             # truly flushed correctly.
             assert self._collection_type.get_current_buffer_size() == 0
 
-    @pytest.mark.xfail(
+    @pytest.mark.skipif(
+        ON_WINDOWS,
         reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
         ),
     )
     def test_multithreaded_buffering_setitem(self, tmpdir):
@@ -392,11 +395,11 @@ class TestBufferedJSONDict(BufferedJSONCollectionTest, TestJSONDict):
 
         self.multithreaded_buffering_test(setitem_dict, tmpdir)
 
-    @pytest.mark.xfail(
+    @pytest.mark.skipif(
+        ON_WINDOWS,
         reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
         ),
     )
     def test_multithreaded_buffering_update(self, tmpdir):
@@ -407,11 +410,11 @@ class TestBufferedJSONDict(BufferedJSONCollectionTest, TestJSONDict):
 
         self.multithreaded_buffering_test(update_dict, tmpdir)
 
-    @pytest.mark.xfail(
+    @pytest.mark.skipif(
+        ON_WINDOWS,
         reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
         ),
     )
     def test_multithreaded_buffering_reset(self, tmpdir):
@@ -422,11 +425,11 @@ class TestBufferedJSONDict(BufferedJSONCollectionTest, TestJSONDict):
 
         self.multithreaded_buffering_test(reset_dict, tmpdir)
 
-    @pytest.mark.xfail(
+    @pytest.mark.skipif(
+        ON_WINDOWS,
         reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
         ),
     )
     def test_multithreaded_buffering_clear(self, tmpdir):
@@ -474,11 +477,11 @@ class TestBufferedJSONDict(BufferedJSONCollectionTest, TestJSONDict):
             # truly flushed correctly.
             assert self._collection_type.get_current_buffer_size() == 0
 
-    @pytest.mark.xfail(
+    @pytest.mark.skipif(
+        ON_WINDOWS,
         reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
         ),
     )
     def test_multithreaded_buffering_load(self, tmpdir):
@@ -670,11 +673,11 @@ class TestBufferedJSONList(BufferedJSONCollectionTest, TestJSONList):
             # truly flushed correctly.
             assert self._collection_type.get_current_buffer_size() == 0
 
-    @pytest.mark.xfail(
+    @pytest.mark.skipif(
+        ON_WINDOWS,
         reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
         ),
     )
     def test_multithreaded_buffering_setitem(self, tmpdir):
@@ -686,11 +689,11 @@ class TestBufferedJSONList(BufferedJSONCollectionTest, TestJSONList):
 
         self.multithreaded_buffering_test(setitem_list, True, tmpdir)
 
-    @pytest.mark.xfail(
+    @pytest.mark.skipif(
+        ON_WINDOWS,
         reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
         ),
     )
     def test_multithreaded_buffering_extend(self, tmpdir):
@@ -701,11 +704,11 @@ class TestBufferedJSONList(BufferedJSONCollectionTest, TestJSONList):
 
         self.multithreaded_buffering_test(extend_list, False, tmpdir)
 
-    @pytest.mark.xfail(
+    @pytest.mark.skipif(
+        ON_WINDOWS,
         reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
         ),
     )
     def test_multithreaded_buffering_append(self, tmpdir):
@@ -717,11 +720,11 @@ class TestBufferedJSONList(BufferedJSONCollectionTest, TestJSONList):
 
         self.multithreaded_buffering_test(append_list, False, tmpdir)
 
-    @pytest.mark.xfail(
+    @pytest.mark.skipif(
+        ON_WINDOWS,
         reason=(
-            "This test sometimes fails. This may indicate a race condition. "
-            "The test fails more consistently on Windows but also appears on "
-            "Linux in CI."
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
         ),
     )
     def test_multithreaded_buffering_load(self, tmpdir):
diff --git a/tests/test_synced_collections/test_json_collection.py b/tests/test_synced_collections/test_json_collection.py
index a32ea6fe..2420ceea 100644
--- a/tests/test_synced_collections/test_json_collection.py
+++ b/tests/test_synced_collections/test_json_collection.py
@@ -9,6 +9,7 @@ from attr_dict_test import AttrDictTest, AttrListTest
 from synced_collection_test import SyncedDictTest, SyncedListTest
 
 from signac._synced_collections.backends.collection_json import (
+    ON_WINDOWS,
     JSONAttrDict,
     JSONAttrList,
     JSONDict,
@@ -42,6 +43,16 @@ class JSONCollectionTest:
     def test_filename(self, synced_collection):
         assert os.path.basename(synced_collection.filename) == self._fn
 
+    @pytest.mark.skipif(
+        ON_WINDOWS,
+        reason=(
+            "The JSONCollection cannot be safely used in multithreaded settings "
+            "on Windows due to https://bugs.python.org/issue46003."
+        ),
+    )
+    def test_multithreaded(self, synced_collection):
+        return super().test_multithreaded(synced_collection)
+
 
 class TestJSONDict(JSONCollectionTest, SyncedDictTest):