Avoid deadlock in SyncedCollection

[joaander]

Description

Add locks needed to avoid deadlocks in SyncedCollection

Motivation and Context

This MWE deadlocks because SycedCollections._locks[lock_id] gets overwritten while it is being used:

import json
import tempfile
from concurrent.futures import ThreadPoolExecutor, ProcessPoolExecutor
from signac.synced_collections.backends.collection_json import JSONAttrDict
def update_status(data):
    s, filename = data
    d = JSONAttrDict(filename=filename)
    d.update(dict(a=1, b=s-1))
    return False
if __name__ == "__main__":
    N = 1
    n_tasks = 20
    with tempfile.TemporaryDirectory() as d:
        for s in range(N):
            with open(f'{d}/{s}.json', 'w') as f:
                json.dump(dict(a=s, b=0), f)
        tasks = []
        for s in range(N):
            for t in range(n_tasks):
                tasks.append((s, f'{d}/{s}.json'))
        with ThreadPoolExecutor() as e:
            e.map(update_status, tasks)
        print(JSONAttrDict._locks)

Types of Changes

• Documentation update
• Bug fix
• New feature
• Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

• I am familiar with the Contributing Guidelines.
• I agree with the terms of the Contributor Agreement.
• My name is on the list of contributors.
• My code follows the code style guideline 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 on signac-docs and linked it here.
• I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

[codecov]

Codecov Report

Merging #530 (21f11de) into master (786d75f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #530   +/-   ##
=======================================
  Coverage   78.16%   78.17%           
=======================================
  Files          65       65           
  Lines        7036     7039    +3     
  Branches     1310     1311    +1     
=======================================
+ Hits         5500     5503    +3     
  Misses       1231     1231           
  Partials      305      305           

Impacted Files	Coverage Δ
...synced_collections/data_types/synced_collection.py	92.14% <100.00%> (+0.17%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 786d75f...21f11de. Read the comment docs.

[bdice]

Looks good to me. Thanks again for your help, @joaander. If you can think of a test that would fail (but not deadlock), that would be a good addition. I don't know how I would construct such a case unless there is a way to force a timeout.

[joaander]

Looks good to me. Thanks again for your help, @joaander. If you can think of a test that would fail (but not deadlock), that would be a good addition. I don't know how I would construct such a case unless there is a way to force a timeout.

I tried to get failing behavior in my first attempt on the way to this MWE but wan't successful. I had tried to trigger conflicting writes to the file.

If inspection of private variables is allowed in the test, a test that would fail before this change would construct two JSONAttrDicts with the same file and assert that the identify of the lock JSONAttrDict._locks[filename] stays the same.

[cbkerr]

With help from @bdice, I was able to reproduce the problem and fix even though I didn't experience a deadlock by printing threading.get_ident() and "done" inside the update_status function.

[vyasr]

In the interest of getting things merged I'll look into constructing a test later.