BF: interprocess lock for checking/opening ssh socket connections #5466
Conversation
This should prevent race condition between testing if socket path already exists and establishing it. Closes: datalad#5460 Related issues discovered while troubleshooting for this one are datalad#5465 datalad#5464
|
@bpoldrack @mih you might like to test this on your ssh use cases to see if it is not adding a notable slow down -- our benchmarking wouldn't test for this. |
|
ha -- travis hits old #4101 (and again on maint). appveyor is just unhappy and this time against more than those Macs... even ubuntu freaks out. |
datalad/support/sshconnector.py
Outdated
| @@ -521,6 +521,12 @@ def open(self): | |||
| ConnectionOpenFailedError | |||
| When starting the SSH ControlMaster process failed. | |||
| """ | |||
| with fasteners.process_lock.InterProcessLock(self.ctrl_path.with_suffix('.lck')): | |||
There was a problem hiding this comment.
Could datalad.support.locking.lock_if_check_fails be used here to avoid creating a lock in many cases?
There was a problem hiding this comment.
oh right
- we should just have a lock instance bound to the instance and reuse it (should shave off some cycles).
- Indeed we should be able to use
lock_if_check_failsbut it would loose that (reuse instance) so I might need to RF it to allow to reuse a lock provided from outside to take advantage of1.
There was a problem hiding this comment.
Decided against using lock_if_check_fails: it was coded up with use cases in mind where check would check for something you expect to not disappear (e.g. downloaded annex key for an archive) between you checking and then doing some next action... so for faster paced use cases I am afraid there might be a race condition that we would happily not bother establishing the socket while it disappears from us. So, to be safer than sorry decided to just always lock so we do not get returned upon file check until some process establishes it (although I think my logic above is flawed and not using it would not give us any protection but at least code would be easier to follow and I do not think it would really make a big difference in terms of performance)
I think there might still be benefit from reusing a lock instance so I have pushed a RF which does that while relying on a feature of fasteners.locked to have a list of locks instead of a single one. So the diff becomes shorter ;)
…" feature of fasteners I believe multi-lock feature was there since forever or exactly since when .locked was added in 0.7.0~3^2~8 so no boost in version for fasteners is needed
Codecov Report
@@ Coverage Diff @@
## maint #5466 +/- ##
==========================================
+ Coverage 89.54% 90.11% +0.57%
==========================================
Files 296 296
Lines 41790 41829 +39
==========================================
+ Hits 37420 37696 +276
+ Misses 4370 4133 -237
Continue to review full report at Codecov.
|
| self._lock = [ | ||
| threading.Lock(), | ||
| fasteners.process_lock.InterProcessLock(self.ctrl_path.with_suffix('.lck')) | ||
| ] |
There was a problem hiding this comment.
Oh, that's fancy. Thanks for leaving the comment to point future confused readers in the right direction.
This should prevent race condition between testing if socket path
already exists and establishing it.
Closes: #5460
Related issues discovered while troubleshooting for this one are
#5465
#5464