Add semaphore to AsyncFileSystemWrapper #1901
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Initialize a semaphore in AsyncFileSystemWrapper if asynchronous mode is disabled. This ensures that concurrent calls are managed safely, preventing potential deadlocks in systems that cannot manage concurrent requests. - Added LockedFileSystem class to test this case.
|
Your implementation is interesting - it allows for the amount of concurrency to be controlled; it should be surfaced to the whole wrapper and disabled by default; I suspect that in test_semaphore_synchronous with a semaphore with large value or none at all, you would NOT see a deadlock. So, I think that allowing to pass in a semaphore in a fine idea, but I'm not sure that defaulting to 1 is a good idea. I don't know if it really has a negative consequence, but we already knew that LocalFileSystem didn't have any deadlock issues. You mock has to explicitly make a lock to create a problem :) See this ancient discussion of the strange things that paramiko in particular does https://stackoverflow.com/a/32875571/3821154 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request tries to enhance the
AsyncFileSystemWrapperby introducing semaphore support to limit concurrent calls to the underlying synchronous file system. These changes would address the issues encountered while using zarr v3 withfsspec.implementations.sftp, as discussed in Zarr issue #3196.Perhaps I am missing the bigger picture here and this is actually is not a good idea – so any thoughts or feedback is appreciated :)
Suggested changes