async_ssh: Use async semaphore instead of manual locking #7018
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7018 +/- ##
==========================================
- Coverage 79.10% 79.10% -0.00%
==========================================
Files 567 567
Lines 43803 43793 -10
==========================================
- Hits 34648 34638 -10
Misses 9155 9155 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
danielhollas
force-pushed
the
async-semaphore
branch
from
af1042c to
f9a3503
Compare
| async with self._semaphore: | ||
| try: | ||
| await self.async_backend.put( | ||
| localpath=localpath, | ||
| remotepath=remotepath, | ||
| dereference=dereference, | ||
| preserve=preserve, | ||
| recursive=False, | ||
| ) | ||
| except OSError as exc: | ||
| raise OSError(f'Error while uploading file {localpath}: {exc}') |
There was a problem hiding this comment.
Just look at the code, the equivalent change should be put async with _semaphore inside the try block right before the put().
But I think it is a correct change.
pinning @khsrali for review since you implement it.
There was a problem hiding this comment.
Okay, I overlooked what @danielhollas already wrote: "nd fixes a bug since the number of file IO connections was not decreased when an OsError exception was thrown.".
|
@unkcpz @khsrali thank you for the review! @khsrali do you think you could benchmark this PR in the same way you benchmarked the original async_ssh implemetation to see if this fixes the regression you saw for small files? I am planning to write a regression test here to make sure we're releasing the resources in case of exceptions. |
3 participants
This should improve performance of async_ssh plugin for lots of small files since we're no longer sleeping 500ms when waiting for a lock.
https://docs.python.org/3/library/asyncio-sync.html#asyncio.Semaphore
Also makes the code more safe since one doesn't need to manually lock and unlock, and fixes a bug since the number of file IO connections was not decreased when an OsError exception was thrown.