Repository: glotzerlab/signac
Pull request #466 (2021-01-06 03:01:47)

Feature/synced collection/cleanup
<!-- Provide a general summary of your changes in the Title above -->

## Description
<!-- Describe your changes in detail -->

Address the remaining TODO items throughout the synced_collections code base, except for the one that's relevant to 
#397.

## Types of Changes
<!-- Please select all items that apply either now or after creating the pull request: -->
- [ ] Documentation update
- [ ] Bug fix
- [ ] New feature
- [ ] Breaking change<sup>1</sup>

<sup>1</sup>The change breaks (or has the potential to break) existing functionality.

## Checklist:
<!-- Please select all items that apply either now or after creating the pull request. -->
<!-- If you are unsure about any of these items, do not hesitate to ask! -->
- [ ] I am familiar with the [**Contributing Guidelines**](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [ ] I agree with the terms of the [**Contributor Agreement**](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [ ] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [ ] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [ ] The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:
- [ ] I have updated the API documentation as part of the package doc-strings.
- [ ] I have created a separate pull request to update the [framework documentation](https://docs.signac.io/) on [signac-docs](https://github.com/glotzerlab/signac-docs) and linked it here.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added all related issue and pull request numbers for future reference (if applicable). See example below.

<!-- Example for a changelog entry: `Fix issue with launching rockets to the moon (#101, #212).` -->


codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/466?src=pr&el=h1) Report
> Merging [#466](https://codecov.io/gh/glotzerlab/signac/pull/466?src=pr&el=desc) (9ea41da) into [feature/synced_collections](https://codecov.io/gh/glotzerlab/signac/commit/af991e772247490f0f184def71ba34b213dc97d2?el=desc) (af991e7) will **increase** coverage by `0.09%`.
> The diff coverage is `92.30%`.

[![Impacted file tree graph](https://codecov.io/gh/glotzerlab/signac/pull/466/graphs/tree.svg?width=650&height=150&src=pr&token=FghbjTyzgM)](https://codecov.io/gh/glotzerlab/signac/pull/466?src=pr&el=tree)

```diff
@@                      Coverage Diff                       @@
##           feature/synced_collections     #466      +/-   ##
==============================================================
+ Coverage                       78.75%   78.85%   +0.09%     
==============================================================
  Files                              57       57              
  Lines                            6741     6749       +8     
  Branches                         1253     1253              
==============================================================
+ Hits                             5309     5322      +13     
+ Misses                           1126     1123       -3     
+ Partials                          306      304       -2     
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/466?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [signac/core/synced\_collections/utils.py](https://codecov.io/gh/glotzerlab/signac/pull/466/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkX2NvbGxlY3Rpb25zL3V0aWxzLnB5) | `72.50% <ø> (ø)` | |
| [signac/core/synced\_collections/collection\_zarr.py](https://codecov.io/gh/glotzerlab/signac/pull/466/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkX2NvbGxlY3Rpb25zL2NvbGxlY3Rpb25femFyci5weQ==) | `91.30% <75.00%> (-3.70%)` | :arrow_down: |
| [signac/core/synced\_collections/collection\_json.py](https://codecov.io/gh/glotzerlab/signac/pull/466/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkX2NvbGxlY3Rpb25zL2NvbGxlY3Rpb25fanNvbi5weQ==) | `98.91% <100.00%> (+3.26%)` | :arrow_up: |
| [...gnac/core/synced\_collections/collection\_mongodb.py](https://codecov.io/gh/glotzerlab/signac/pull/466/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkX2NvbGxlY3Rpb25zL2NvbGxlY3Rpb25fbW9uZ29kYi5weQ==) | `100.00% <100.00%> (ø)` | |
| [signac/core/synced\_collections/collection\_redis.py](https://codecov.io/gh/glotzerlab/signac/pull/466/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkX2NvbGxlY3Rpb25zL2NvbGxlY3Rpb25fcmVkaXMucHk=) | `100.00% <100.00%> (+5.71%)` | :arrow_up: |
| [...ore/synced\_collections/file\_buffered\_collection.py](https://codecov.io/gh/glotzerlab/signac/pull/466/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkX2NvbGxlY3Rpb25zL2ZpbGVfYnVmZmVyZWRfY29sbGVjdGlvbi5weQ==) | `94.69% <100.00%> (+0.14%)` | :arrow_up: |
| [...e/synced\_collections/memory\_buffered\_collection.py](https://codecov.io/gh/glotzerlab/signac/pull/466/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkX2NvbGxlY3Rpb25zL21lbW9yeV9idWZmZXJlZF9jb2xsZWN0aW9uLnB5) | `91.17% <100.00%> (+0.26%)` | :arrow_up: |
| [...ignac/core/synced\_collections/synced\_collection.py](https://codecov.io/gh/glotzerlab/signac/pull/466/diff?src=pr&el=tree#diff-c2lnbmFjL2NvcmUvc3luY2VkX2NvbGxlY3Rpb25zL3N5bmNlZF9jb2xsZWN0aW9uLnB5) | `91.94% <100.00%> (+1.34%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/466?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/466?src=pr&el=footer). Last update [af991e7...9ea41da](https://codecov.io/gh/glotzerlab/signac/pull/466?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/doc/api.rst b/doc/api.rst
index 13de49dc..64aaa799 100644
--- a/doc/api.rst
+++ b/doc/api.rst
@@ -208,6 +208,22 @@ synced\_collections.buffered\_collection module
    :undoc-members:
    :show-inheritance:
 
+synced\_collections.file\_buffered\_collection module
+-----------------------------------------------
+
+.. automodule:: signac.core.synced_collections.file_buffered_collection
+   :members:
+   :undoc-members:
+   :show-inheritance:
+
+synced\_collections.memory\_buffered\_collection module
+-----------------------------------------------
+
+.. automodule:: signac.core.synced_collections.memory_buffered_collection
+   :members:
+   :undoc-members:
+   :show-inheritance:
+
 synced\_collections.caching module
 ----------------------------------
 
@@ -247,3 +263,11 @@ synced\_collections.collection\_zarr module
    :members:
    :undoc-members:
    :show-inheritance:
+
+synced\_collections.utils module
+-------------------------------------------
+
+.. automodule:: signac.core.synced_collections.utils
+   :members:
+   :undoc-members:
+   :show-inheritance:
diff --git a/signac/core/synced_collections/collection_json.py b/signac/core/synced_collections/collection_json.py
index df638f74..02d2600c 100644
--- a/signac/core/synced_collections/collection_json.py
+++ b/signac/core/synced_collections/collection_json.py
@@ -112,7 +112,7 @@ class JSONCollection(SyncedCollection):
         # replace that file with original file. We also enable this mode
         # irrespective of the write_concern flag if we're running in
         # multithreaded mode.
-        if self._write_concern or hasattr(type(self), "_locks"):
+        if self._write_concern or type(self)._threading_support_is_active:
             dirname, filename = os.path.split(self._filename)
             fn_tmp = os.path.join(dirname, f"._{uuid.uuid4()}_{filename}")
             with open(fn_tmp, "wb") as tmpfile:
diff --git a/signac/core/synced_collections/collection_mongodb.py b/signac/core/synced_collections/collection_mongodb.py
index 9f2f3164..ab86de0d 100644
--- a/signac/core/synced_collections/collection_mongodb.py
+++ b/signac/core/synced_collections/collection_mongodb.py
@@ -2,8 +2,6 @@
 # All rights reserved.
 # This software is licensed under the BSD 3-Clause License.
 """Implements a MongoDB SyncedCollection backend."""
-from copy import deepcopy
-
 import bson
 
 from .synced_attr_dict import SyncedAttrDict
@@ -68,19 +66,6 @@ class MongoDBCollection(SyncedCollection):
         except bson.errors.InvalidDocument as err:
             raise TypeError(str(err))
 
-    def _pseudo_deepcopy(self):
-        """Return a copy of instance.
-
-        It is a pseudo implementation for `deepcopy` because
-        :py:class:`pymongo.collection.Collection` does not support `deepcopy` method.
-        """
-        return type(self)(
-            collection=self._collection,
-            uid=self._uid,
-            data=self._to_base(),
-            parent=deepcopy(self._parent),
-        )
-
     @property
     def collection(self):
         """pymongo.collection.Collection: Get the collection being synced to."""
@@ -91,6 +76,10 @@ class MongoDBCollection(SyncedCollection):
         """dict: Get the unique mapping used to identify this collection."""
         return self._uid
 
+    def __deepcopy__(self, memo):
+        # The underlying MongoDB collection cannot be deepcopied.
+        raise TypeError("MongoDBCollection does not support deepcopying.")
+
 
 class MongoDBDict(MongoDBCollection, SyncedAttrDict):
     """A dict-like mapping interface to a persistent document in a MongoDB collection.
@@ -116,10 +105,10 @@ class MongoDBDict(MongoDBCollection, SyncedAttrDict):
 
     While the MongoDBDict object behaves like a dictionary, there are important
     distinctions to remember. In particular, because operations are reflected
-    as changes to an underlying database, copying (even deep copying) a
-    MongoDBDict instance may exhibit unexpected behavior. If a true copy is
-    required, you should use the call operator to get a dictionary
-    representation, and if necessary construct a new MongoDBDict instance:
+    as changes to an underlying database, copying a MongoDBDict instance may
+    exhibit unexpected behavior. If a true copy is required, you should use the
+    call operator to get a dictionary representation, and if necessary
+    construct a new MongoDBDict instance:
     ``new_dict = MongoDBDict(old_dict())``.
 
     Parameters
@@ -173,10 +162,10 @@ class MongoDBList(MongoDBCollection, SyncedList):
 
     While the MongoDBList object behaves like a list, there are important
     distinctions to remember. In particular, because operations are reflected
-    as changes to an underlying database, copying (even deep copying) a
-    MongoDBList instance may exhibit unexpected behavior. If a true copy is
-    required, you should use the call operator to get a dictionary
-    representation, and if necessary construct a new MongoDBList instance:
+    as changes to an underlying database, copying a MongoDBList instance may
+    exhibit unexpected behavior. If a true copy is required, you should use the
+    call operator to get a dictionary representation, and if necessary
+    construct a new MongoDBList instance:
     ``new_list = MongoDBList(old_list())``.
 
     """
diff --git a/signac/core/synced_collections/collection_redis.py b/signac/core/synced_collections/collection_redis.py
index 90b1a188..f2411c09 100644
--- a/signac/core/synced_collections/collection_redis.py
+++ b/signac/core/synced_collections/collection_redis.py
@@ -3,7 +3,6 @@
 # This software is licensed under the BSD 3-Clause License.
 """Implements a Redis SyncedCollection backend."""
 import json
-from copy import deepcopy
 
 from .synced_attr_dict import SyncedAttrDict
 from .synced_collection import SyncedCollection
@@ -48,26 +47,6 @@ class RedisCollection(SyncedCollection):
         """Write the data to a Redis database."""
         self._client.set(self._key, json.dumps(self._to_base()).encode())
 
-    def _pseudo_deepcopy(self):
-        """Return a copy of instance.
-
-        It is a pseudo implementation for `deepcopy` because
-        `redis.Redis` does not support `deepcopy` method.
-        """
-        if self._parent is not None:
-            # TODO: Do we really want a deep copy of a nested collection to
-            # deep copy the parent? Perhaps we should simply disallow this?
-            return type(self)(
-                client=None,
-                key=None,
-                data=self._to_base(),
-                parent=deepcopy(self._parent),
-            )
-        else:
-            return type(self)(
-                client=self._client, key=self._key, data=None, parent=None
-            )
-
     @property
     def client(self):
         """`redis.Redis`: The Redis client used to store the data."""
@@ -78,6 +57,10 @@ class RedisCollection(SyncedCollection):
         """str: The key associated with this collection stored in Redis."""
         return self._key
 
+    def __deepcopy__(self, memo):
+        # The underlying Redis client cannot be deepcopied.
+        raise TypeError("RedisCollection does not support deepcopying.")
+
 
 class RedisDict(RedisCollection, SyncedAttrDict):
     """A dict-like mapping interface to a persistent Redis-database.
@@ -114,11 +97,10 @@ class RedisDict(RedisCollection, SyncedAttrDict):
 
     While the RedisDict object behaves like a dictionary, there are important
     distinctions to remember. In particular, because operations are reflected
-    as changes to an underlying database, copying (even deep copying) a
-    RedisDict instance may exhibit unexpected behavior. If a true copy is
-    required, you should use the call operator to get a dictionary
-    representation, and if necessary construct a new RedisDict instance:
-    ``new_dict = RedisDict(old_dict())``.
+    as changes to an underlying database, copying a RedisDict instance may
+    exhibit unexpected behavior. If a true copy is required, you should use the
+    call operator to get a dictionary representation, and if necessary
+    construct a new RedisDict instance: ``new_dict = RedisDict(old_dict())``.
 
     """
 
@@ -144,11 +126,11 @@ class RedisList(RedisCollection, SyncedList):
 
         While the RedisList object behaves like a list, there are
         important distinctions to remember. In particular, because operations
-        are reflected as changes to an underlying database, copying (even deep
-        copying) a RedisList instance may exhibit unexpected behavior. If a
-        true copy is required, you should use the call operator to get a
-        dictionary representation, and if necessary construct a new RedisList
-        instance: `new_list = RedisList(old_list())`.
+        are reflected as changes to an underlying database, copying a RedisList
+        instance may exhibit unexpected behavior. If a true copy is required,
+        you should use the call operator to get a dictionary representation,
+        and if necessary construct a new RedisList instance:
+        ``new_list = RedisList(old_list())``.
 
     Parameters
     ----------
diff --git a/signac/core/synced_collections/collection_zarr.py b/signac/core/synced_collections/collection_zarr.py
index a27d240a..efccff65 100644
--- a/signac/core/synced_collections/collection_zarr.py
+++ b/signac/core/synced_collections/collection_zarr.py
@@ -4,7 +4,6 @@
 """Implements a Zarr SyncedCollection backend."""
 from copy import deepcopy
 
-# TODO: Give a clearer error if the numcodecs import fails.
 import numcodecs
 
 from .synced_attr_dict import SyncedAttrDict
@@ -26,17 +25,18 @@ class ZarrCollection(SyncedCollection):
         The Zarr group in which to store data.
     name : str
         The name under which this collection is stored in the Zarr group.
+    codec : numcodecs.abc.Codec
+        The encoding mechanism for the data. If not provided, defaults to JSON
+        encoding (Default value: None).
 
     """
 
     _backend = __name__  # type: ignore
 
-    def __init__(self, group=None, name=None, **kwargs):
+    def __init__(self, group=None, name=None, codec=None, **kwargs):
         self._root = group
-        # TODO: Give users control over the codec. If we use JSON encoding,
-        # then we should also use the JSON validator.
-        self._object_codec = numcodecs.JSON()
         self._name = name
+        self._object_codec = numcodecs.JSON() if codec is None else codec
         super().__init__(**kwargs)
 
     def _load_from_resource(self):
@@ -68,8 +68,6 @@ class ZarrCollection(SyncedCollection):
 
     def __deepcopy__(self, memo):
         if self._parent is not None:
-            # TODO: Do we really want a deep copy of a nested collection to
-            # deep copy the parent? Perhaps we should simply disallow this?
             return type(self)(
                 group=None,
                 name=None,
@@ -84,6 +82,15 @@ class ZarrCollection(SyncedCollection):
                 parent=None,
             )
 
+    @property
+    def codec(self):
+        """numcodecs.abc.Codec: The encoding method used for the data."""
+        return self._object_codec
+
+    @codec.setter
+    def codec(self, new_codec):
+        self._object_codec = new_codec
+
     @property
     def group(self):
         """zarr.hierarchy.Group: The Zarr group storing the data."""
diff --git a/signac/core/synced_collections/file_buffered_collection.py b/signac/core/synced_collections/file_buffered_collection.py
index 47dd3834..a8d9a3c9 100644
--- a/signac/core/synced_collections/file_buffered_collection.py
+++ b/signac/core/synced_collections/file_buffered_collection.py
@@ -47,6 +47,18 @@ class FileBufferedCollection(BufferedCollection):
         process, but this is dependent on its constructor being called before
         those of other classes.
 
+    Warnings
+    --------
+    Although it can be done safely, in general modifying two different collections
+    pointing to the same underlying resource while both are in different buffering
+    modes is unsupported and can lead to undefined behavior. This class makes a
+    best effort at performing safe modifications, but it is possible to construct
+    nested buffered contexts for different objects that can lead to an invalid
+    buffer state, or even situations where there is no obvious indicator of what
+    is the canonical source of truth. In general, if you need multiple objects
+    pointing to the same resource, it is **strongly** recommeneded to work with
+    both of them in identical buffering states at all times.
+
     """
 
     # Note for developers: since all subclasses share a single cache, all
@@ -295,6 +307,15 @@ class FileBufferedCollection(BufferedCollection):
             if id(self) not in FileBufferedCollection._cached_collections:
                 FileBufferedCollection._cached_collections[id(self)] = self
         else:
+            # The first time this method is called, if nothing is in the buffer
+            # for this file then we cannot guarantee that the _data attribute
+            # is valid either since the resource could have been modified
+            # between when _data was last updated and when this load is being
+            # called. As a result, we have to load from the resource here to be
+            # safe.
+            data = self._load_from_resource()
+            with self._suspend_sync():
+                self._update(data)
             self._initialize_data_in_cache()
 
         # Load from buffer
diff --git a/signac/core/synced_collections/memory_buffered_collection.py b/signac/core/synced_collections/memory_buffered_collection.py
index 0507b843..77ceda53 100644
--- a/signac/core/synced_collections/memory_buffered_collection.py
+++ b/signac/core/synced_collections/memory_buffered_collection.py
@@ -77,16 +77,22 @@ class SharedMemoryFileBufferedCollection(BufferedCollection):
         process, but this is dependent on its constructor being called before
         those of other classes.
 
+    Warnings
+    --------
+    Although it can be done safely, in general modifying two different collections
+    pointing to the same underlying resource while both are in different buffering
+    modes is unsupported and can lead to undefined behavior. This class makes a
+    best effort at performing safe modifications, but it is possible to construct
+    nested buffered contexts for different objects that can lead to an invalid
+    buffer state, or even situations where there is no obvious indicator of what
+    is the canonical source of truth. In general, if you need multiple objects
+    pointing to the same resource, it is **strongly** recommeneded to work with
+    both of them in identical buffering states at all times.
+
     """
 
     _cache: Dict[str, Dict[str, Union[bytes, str, Tuple[int, float, int]]]] = {}
     _cached_collections: Dict[int, BufferedCollection] = {}
-    # TODO: Do we really care about the total number of objects stored in the
-    # buffer, or do we only care about objects that have been modified (and
-    # therefore require a file write when flushing)? For files that have been
-    # read but not modified, I don't think that there's any reason not to just
-    # let them sit in memory, or at least to give the user an option to allow
-    # that in case they know that virtual memory exhaustion won't be an issue.
     _BUFFER_CAPACITY = 1000  # The number of collections to store in the buffer.
     _CURRENT_BUFFER_SIZE = 0
 
@@ -180,9 +186,6 @@ class SharedMemoryFileBufferedCollection(BufferedCollection):
 
         """
         if not self._is_buffered or force:
-            # TODO: If we have two objects pointing to the same filename in the
-            # cache and one of them flushes before the other, need to decide
-            # how to handle it.
             try:
                 _, stored_metadata = self._cached_collections.pop(id(self))
                 cached_data = self._cache[self._filename]
@@ -283,11 +286,15 @@ class SharedMemoryFileBufferedCollection(BufferedCollection):
                     self._cache[self._filename]["metadata"],
                 )
         else:
-            # TODO: The first time we call _load_from_buffer we might need to call
-            # _load_from_resource. Otherwise, if something modified the file in memory
-            # since the last time that we performed any save/load operation, we could be
-            # putting an out-of-date state into the buffer. This also affects the
-            # FileBufferedCollection.
+            # The first time this method is called, if nothing is in the buffer
+            # for this file then we cannot guarantee that the _data attribute
+            # is valid either since the resource could have been modified
+            # between when _data was last updated and when this load is being
+            # called. As a result, we have to load from the resource here to be
+            # safe.
+            data = self._load_from_resource()
+            with self._suspend_sync():
+                self._update(data)
             self._initialize_data_in_cache(modified=False)
 
         # Set local data to the version in the buffer.
diff --git a/signac/core/synced_collections/synced_collection.py b/signac/core/synced_collections/synced_collection.py
index ef706dd3..9394ec29 100644
--- a/signac/core/synced_collections/synced_collection.py
+++ b/signac/core/synced_collections/synced_collection.py
@@ -116,6 +116,8 @@ class SyncedCollection(Collection):
     def __init__(self, parent=None, *args, **kwargs):
         self._parent = parent
         self._suspend_sync_ = 0
+        if type(self)._supports_threading:
+            type(self)._locks[self._lock_id] = RLock()
 
     @classmethod
     def __init_subclass__(cls):
@@ -137,7 +139,10 @@ class SyncedCollection(Collection):
 
         # Monkey-patch subclasses that support locking.
         if cls._supports_threading:
+            cls._locks = {}
             cls.enable_multithreading()
+        else:
+            cls.disable_multithreading()
 
     @classmethod
     def enable_multithreading(cls):
@@ -148,8 +153,8 @@ class SyncedCollection(Collection):
 
         """
         if cls._supports_threading:
-            cls._locks = defaultdict(RLock)
             cls._thread_lock = _thread_lock
+            cls._threading_support_is_active = True
         else:
             raise ValueError("This class does not support multithreaded execution.")
 
@@ -161,14 +166,8 @@ class SyncedCollection(Collection):
         costs, so they can be disabled for classes that support it.
 
         """
-        try:
-            del cls._locks
-            cls._thread_lock = _fake_lock
-        except AttributeError:
-            raise ValueError("This class does not support multithreaded execution.")
-
-    # By default, classes do not support locking.
-    _thread_lock = _fake_lock
+        cls._thread_lock = _fake_lock
+        cls._threading_support_is_active = False
 
     @property
     def validators(self):
@@ -176,11 +175,6 @@ class SyncedCollection(Collection):
 
         Validators are inherited from all parents of a class.
         """
-        # TODO: Determine whether it makes sense to construct this list here,
-        # or whether we can just do it at initialization and cache it. The only
-        # reason not to do that would be to support adding validators to a
-        # class after instantiating objects and still having those validators
-        # applied, which I don't think is necessary.
         validators = []
         # Classes inherit the validators of their parent classes.
         for base_cls in type(self).__mro__:
@@ -256,8 +250,6 @@ class SyncedCollection(Collection):
         if NUMPY:
             if isinstance(data, numpy.number):
                 return data.item()
-        # TODO: This return value could be the original object if no match is
-        # found, there should be an error or at least a warning.
         return data
 
     @abstractmethod
diff --git a/signac/core/synced_collections/utils.py b/signac/core/synced_collections/utils.py
index 1c7cecd0..b0a7a324 100644
--- a/signac/core/synced_collections/utils.py
+++ b/signac/core/synced_collections/utils.py
@@ -93,9 +93,14 @@ def default(o: Any) -> Dict[str, Any]:  # noqa: D102
 
     Warnings
     --------
-    JSON encoding of numpy arrays is not invertible; once encoded, reloading
-    the data will result in converting arrays to lists and numpy numbers into
-    ints or floats.
+    - JSON encoding of numpy arrays is not invertible; once encoded, reloading
+      the data will result in converting arrays to lists and numpy numbers into
+      ints or floats.
+    - This function assumes that the in-memory data for a SyncedCollection is
+      up-to-date. If the data has been changed on disk without updating the
+      collection, or if this function is used to serialize the data before any
+      method is invoked that would load the data from disk, the resulting
+      serialized data may be incorrect.
 
     """
     if NUMPY:
@@ -118,17 +123,17 @@ class SCJSONEncoder(JSONEncoder):
 
     Warnings
     --------
-    JSON encoding of numpy arrays is not invertible; once encoded, reloading
-    the data will result in converting arrays to lists and numpy numbers into
-    ints or floats.
+    - JSON encoding of numpy arrays is not invertible; once encoded, reloading
+      the data will result in converting arrays to lists and numpy numbers into
+      ints or floats.
+    - This class assumes that the in-memory data for a SyncedCollection is
+      up-to-date. If the data has been changed on disk without updating the
+      collection, or if this class is used to serialize the data before any
+      method of the collection is invoked that would load the data from disk,
+      the resulting serialized data may be incorrect.
 
     """
 
-    # TODO: If a user tries to access this encoder to manually dump and calls a
-    # dump before any operation, the data won't have been initialized. This
-    # isn't in itself important, since we'll make this private, but consider
-    # whether there are any issues with that. I assume not, since we're
-    # considering making these objects lazy altogether.
     def default(self, o: Any) -> Dict[str, Any]:  # noqa: D102
         try:
             return default(o)


Test patch:
diff --git a/tests/test_synced_collections/synced_collection_test.py b/tests/test_synced_collections/synced_collection_test.py
index c054b1e2..28bd0124 100644
--- a/tests/test_synced_collections/synced_collection_test.py
+++ b/tests/test_synced_collections/synced_collection_test.py
@@ -1,7 +1,6 @@
 # Copyright (c) 2020 The Regents of the University of Michigan
 # All rights reserved.
 # This software is licensed under the BSD 3-Clause License.
-import platform
 from collections.abc import MutableMapping, MutableSequence
 from copy import deepcopy
 
@@ -18,9 +17,6 @@ except ImportError:
     NUMPY = False
 
 
-PYPY = "PyPy" in platform.python_implementation()
-
-
 class SyncedCollectionTest:
     def store(self, data):
         raise NotImplementedError("All backend tests must implement the store method.")
@@ -287,15 +283,14 @@ class SyncedDictTest(SyncedCollectionTest):
         assert synced_collection() == recursive_convert(synced_collection)
         assert synced_collection() == {"call": testdata}
 
-    @pytest.mark.xfail(reason="Deep copying these objects probably doesn't make sense.")
     def test_reopen(self, synced_collection, testdata):
         key = "reopen"
         synced_collection[key] = testdata
         try:
             synced_collection2 = deepcopy(synced_collection)
         except TypeError:
-            # Use fallback implementation, deepcopy not supported by backend.
-            synced_collection2 = synced_collection._pseudo_deepcopy()
+            # Ignore backends that don't support deepcopy.
+            return
         synced_collection._save()
         del synced_collection  # possibly unsafe
         synced_collection2._load()
@@ -408,6 +403,7 @@ class SyncedDictTest(SyncedCollectionTest):
             return
 
         from concurrent.futures import ThreadPoolExecutor
+        from json.decoder import JSONDecodeError
         from threading import current_thread
 
         def set_value(sd):
@@ -419,29 +415,32 @@ class SyncedDictTest(SyncedCollectionTest):
 
         assert len(synced_collection) == num_threads
 
-        # The ways that PyPy fails here due to what appears to be optimization
-        # of the json library under the hood, so we skip testing the failure
-        # cases on PyPy.
-        if not PYPY:
-            # Now clear the data and try again with multithreading disabled. Unless
-            # we're very unlucky, some of these threads should overwrite each
-            # other.
-            type(synced_collection).disable_multithreading()
-            synced_collection.clear()
+        # Now clear the data and try again with multithreading disabled. Unless
+        # we're very unlucky, some of these threads should overwrite each
+        # other.
+        type(synced_collection).disable_multithreading()
+        synced_collection.clear()
 
+        try:
             with ThreadPoolExecutor(max_workers=num_threads) as executor:
                 list(executor.map(set_value, [synced_collection] * num_threads * 10))
-
+        except (RuntimeError, JSONDecodeError):
+            # This line may raise an exception, or it may successfully complete
+            # but not modify all the expected data. If it raises an exception,
+            # then the underlying data is likely to be invalid, so we must
+            # clear it.
+            synced_collection.clear()
+        else:
             assert len(synced_collection) != num_threads
 
-            # For good measure, try reenabling multithreading and test to be safe.
-            type(synced_collection).enable_multithreading()
-            synced_collection.clear()
+        # For good measure, try reenabling multithreading and test to be safe.
+        type(synced_collection).enable_multithreading()
+        synced_collection.clear()
 
-            with ThreadPoolExecutor(max_workers=num_threads) as executor:
-                list(executor.map(set_value, [synced_collection] * num_threads * 10))
+        with ThreadPoolExecutor(max_workers=num_threads) as executor:
+            list(executor.map(set_value, [synced_collection] * num_threads * 10))
 
-            assert len(synced_collection) == num_threads
+        assert len(synced_collection) == num_threads
 
 
 class SyncedListTest(SyncedCollectionTest):
@@ -613,13 +612,12 @@ class SyncedListTest(SyncedCollectionTest):
         with pytest.raises(ValueError):
             synced_collection._load()
 
-    @pytest.mark.xfail(reason="Deep copying these objects probably doesn't make sense.")
     def test_reopen(self, synced_collection, testdata):
         try:
             synced_collection2 = deepcopy(synced_collection)
         except TypeError:
-            # Use fallback implementation, deepcopy not supported by backend.
-            synced_collection2 = synced_collection._pseudo_deepcopy()
+            # Ignore backends that don't support deepcopy.
+            return
         synced_collection.append(testdata)
         synced_collection._save()
         del synced_collection  # possibly unsafe
diff --git a/tests/test_synced_collections/test_json_buffered_collection.py b/tests/test_synced_collections/test_json_buffered_collection.py
index c9b61a4b..198b3775 100644
--- a/tests/test_synced_collections/test_json_buffered_collection.py
+++ b/tests/test_synced_collections/test_json_buffered_collection.py
@@ -109,22 +109,13 @@ class TestBufferedJSONDict(BufferedJSONCollectionTest, TestJSONDict):
                 assert synced_collection["buffered2"] == 3
 
     def test_two_buffered_modify_unbuffered_first(self, synced_collection, testdata):
-        # TODO: What is the expected behavior in this test? Data is only loaded
-        # into the buffer the first time anything happens, so if we enter the
-        # buffered context for one collection then modify an unbuffered
-        # collection within that context _before_ doing anything with the
-        # buffered collection, the buffered version will overwrite it. This
-        # behavior feels slightly unexpected, but I don't know if there's any
-        # way to fix it. For object-local buffering, we could load into the
-        # buffer when entering the context instead of waiting until the first
-        # call to load, but for global buffering there's no equivalent.
         synced_collection["buffered"] = testdata
         synced_collection2 = self._collection_type(filename=synced_collection._filename)
 
         # Check that the non-buffered object is not modified.
         with synced_collection.buffered():
             synced_collection2["buffered2"] = 1
-            assert "buffered2" not in synced_collection
+            assert "buffered2" in synced_collection
             synced_collection["buffered2"] = 3
         assert synced_collection == {"buffered": testdata, "buffered2": 3}
 
@@ -284,10 +275,10 @@ class TestBufferedJSONDict(BufferedJSONCollectionTest, TestJSONDict):
                 # associated with the metadata stored when synced_collection
                 # entered buffered mode. As a result, when the global buffering
                 # exits, we see metadata errors because synced_collection2 lost
-                # track of the fact that it saved changes to filemade prior to
-                # entering the global buffer. We _could_ fix this by changing
-                # the behavior of _load_buffer to not load the data from the
-                # cache if it exists, if the object is new to
+                # track of the fact that it saved changes to the file made
+                # prior to entering the global buffer. We _could_ fix this by
+                # changing the behavior of _load_buffer to not load the data
+                # from the cache if it exists, if the object is new to
                 # cached_collections then we would save a new version. However,
                 # I'm not sure that's the correct answer. Is there a true
                 # canonical source of truth in this scenario?
@@ -341,6 +332,22 @@ class TestBufferedJSONDict(BufferedJSONCollectionTest, TestJSONDict):
         # Reset buffer capacity for other tests.
         self._collection_type.set_buffer_capacity(original_buffer_capacity)
 
+    def test_buffer_first_load(self, synced_collection):
+        """Ensure that existing data is preserved if the first load is in buffered mode."""
+        fn = synced_collection.filename
+        write_concern = self._write_concern
+
+        sc = self._collection_type(fn, write_concern)
+        sc["foo"] = 1
+        sc["bar"] = 2
+        del sc
+
+        sc = self._collection_type(fn, write_concern)
+        with sc.buffered():
+            sc["foo"] = 3
+
+        assert "bar" in sc
+
 
 class TestBufferedJSONList(BufferedJSONCollectionTest, TestJSONList):
     """Tests of buffering JSONLists."""