-
Notifications
You must be signed in to change notification settings - Fork 66
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
Obtain a lockfile before we write pickle data. #190
Conversation
Codecov Report
@@ Coverage Diff @@
## main #190 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 76 76
Lines 3109 3140 +31
=========================================
+ Hits 3109 3140 +31
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
4c848f5
to
3798823
Compare
@adamgoossens unless i am reading this wrong we are only going to be locking the file just before we write, but if we want to prevent one step overwriting another step, we have to:
currently this can happen:
|
eb7211e
to
ead0c87
Compare
The sequence of events is:
|
075d5ef
to
a742e84
Compare
@adamgoossens its looking really good. just a couple nit pick things. |
Without this it's possible for two concurrent PSR runs to overwrite each other's pickle files on disk. This will result in the artifacts from one of those runs being lost further down the pipeline. This ensures that we: 1) re-read the pickle file from disk before writing out the new one, merging the on-disk data with our in-memory data. 2) adds an exclusive lock around the pickle file to manage concurrent access. We also include a new StepResult.merge method that handles merging two StepResults together if they have the same step name, sub-step name and environment. The StepResult passed to merge takes priority for any duplicate artifact or evidence keys.
a742e84
to
539020e
Compare
@itewk the last of the nits was resolved. I also re-added the use of the Otherwise I think the rest is done. Let me know :) |
@adamgoossens thanks so much. Since this is a more involved/core change I would like @dwinchell to give it a look over too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Without this it's possible for two concurrent PSR runs to overwrite each other's pickle files on disk. This will result in the artifacts from one of those runs being lost further down the pipeline.
This ensures that we: