Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lock lazy properties #529

Merged
merged 7 commits into from
Mar 14, 2021
Merged

Lock lazy properties #529

merged 7 commits into from
Mar 14, 2021

Conversation

bdice
Copy link
Member

@bdice bdice commented Mar 12, 2021

Description

While debugging #528, we identified two sets of changes that are needed for correctness.

This PR adds thread locks around Job and Project methods that lazily instantiate synced collections and HDF5 data stores.

Motivation and Context

Fixes thread safety in Job and Project classes.

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

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

Checklist:

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.

@bdice bdice requested review from a team as code owners March 12, 2021 18:56
@bdice bdice requested review from csadorf, pepak13, vyasr and joaander and removed request for a team, csadorf and pepak13 March 12, 2021 18:56
@mikemhenry
Copy link
Collaborator

Maybe not in this PR -- But can we add some tests so we can catch any regressions RE thread safety?

@codecov
Copy link

codecov bot commented Mar 12, 2021

Codecov Report

Merging #529 (7e69d30) into master (5709232) will increase coverage by 0.09%.
The diff coverage is 89.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #529      +/-   ##
==========================================
+ Coverage   78.17%   78.27%   +0.09%     
==========================================
  Files          65       65              
  Lines        7039     7069      +30     
  Branches     1311     1311              
==========================================
+ Hits         5503     5533      +30     
  Misses       1231     1231              
  Partials      305      305              
Impacted Files Coverage Δ
signac/contrib/job.py 90.55% <87.50%> (+0.58%) ⬆️
signac/contrib/project.py 85.29% <100.00%> (+0.22%) ⬆️

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 5709232...7e69d30. Read the comment docs.

@vyasr
Copy link
Contributor

vyasr commented Mar 14, 2021

Maybe not in this PR -- But can we add some tests so we can catch any regressions RE thread safety?

I think this is a good idea. I've done extensive work to make synced collections thread safe, and there are tests for that, but the thread safety of job objects is far less tested. I'll make an issue for it.

@vyasr vyasr changed the title Lock lazy properties using synced collections. Lock lazy properties Mar 14, 2021
@vyasr vyasr enabled auto-merge (squash) March 14, 2021 15:56
@vyasr vyasr merged commit a20d815 into master Mar 14, 2021
@vyasr vyasr deleted the fix/lazy-locks branch March 14, 2021 16:26
@vyasr vyasr linked an issue Mar 14, 2021 that may be closed by this pull request
@bdice bdice added this to the 1.7.0 milestone Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Incoherent job statepoint access in subprocess
3 participants