Replace TryLock and Wait with Lock, and check for idempotency #7
Conversation
There was a problem hiding this comment.
The couple inline comments are just context for the changes I made to my fork so I could continue testing.
I updated the test code here https://gist.github.com/DisposaBoy/4967b04ea5db2b662562e5b2fcfc5d76
Running without args, it doesn't complete as expected with this PR so I made some changes to fix that in my fork https://github.com/DisposaBoy/certmagic/tree/newlocker.
Running with -crash results in the child progresses hanging while they wait for the stale lock timeout.
|
@DisposaBoy Would you like collaborator privileges so you can push any fixes to my branch and be credited for it in the commit history? Or I can just make the corrections and update this PR. Let me know! |
I don't mind; whichever option's easier for you. |
|
At this point, I think it would be easier for both of us if I just make you a collaborator -- go ahead and push commits to my branch and then we can fix this together. |
…elds like fileStorageNameLocks aren't lost
* Create lockDir just before lock file creation to reduce the chance that another process calls Unlock() and removes lockDir while we were waiting, preventing us from creating the lock file. * Use the same strategy that Wait() uses to avoid depending on internal state.
|
Okay, I've pushed a commit that I think addresses all outstanding issues mentioned in our reviews up to this point. I decided to get rid of the existing notion of the If that optimization is not valuable (i.e. if we can accept many threads polling disk to check the status of the lock), I think we can do away with the Anyway, ready for a "final" review, hopefully. :) |
|
Unfortunately I didn't have much time today so I didn't do a very thorough review but the new code is very similar to what I had in mind. I also wondered if we needed the internal lock ( |
|
Yeah, like I was saying, it's just to reduce extra polling. If the occasional multiple polling is OK, then we can nuke it. |
|
The test app runs fine and nothing else stands out, so apart from my last comment it LGTM. |
I'd personally nuke it. It's always easy to bring it back later if there's a performance issue. |
|
Sounds good -- I'll do that right now, stay tuned (just a few minutes) for the final commit. Then when you change your review to Approved I can merge this! |
|
You're right, this is much better! Thanks for the iterative reviews. This PR also removes almost 30 lines of code! Ready to merge. |
Changes made to be in sync with new storage interface coming from caddyserver/certmagic#7
Changes made to be in sync with new storage interface coming from caddyserver/certmagic#7
This replaces TryLock and Wait with a single Lock function, and we check after obtaining a lock to make sure Obtain and Renew remain idempotent.
See issue #5.
/cc @DisposaBoy - please help test and review!