Skip to content
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

Read/Write File Lock #380

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Read/Write File Lock #380

wants to merge 38 commits into from

Conversation

Yard1
Copy link

@Yard1 Yard1 commented Nov 23, 2024

This PR adds a ReadWriteFileLock API and sync & async implementation (currently only for Unix. I don't know if this is possible on Windows). ReadWriteFileLock allows the caller to operate in either READ or WRITE mode. The lock can be held by multiple READers at the same time, but a WRITEr is guaranteed to have exclusive access to the lock (across both READers and WRITErs). The lock is writer-preferred by employing an outer and inner lock (with the outer serving as staging for acquiring the inner - this means the outer lock is much easier to acquire).

In reader case:

  1. outer lock is acquired non-exclusively
  2. inner lock is acquired non-exclusively
  3. outer lock is released
  4. work is done
  5. inner lock is released

In writer case:

  1. outer lock is acquired exclusively
  2. inner lock is acquired exclusively
  3. work is done
  4. inner lock is released
  5. outer lock is released

While this seems to be working well in practice, there are no actual guarantees of ordering and priority.

I am not firmly settled on the API. Other idea would be to roll wrapper functionality into the base class and provide write_acquire and read_acquire functions (though the context managers would get a bit trickier - one would need to do with self.read_write_lock.read(): instead of simply self.read_write_lock: - as this doesn't allow specifying the mode)

Closes #307

Yard1 and others added 28 commits November 22, 2024 18:26
@Yard1 Yard1 marked this pull request as ready for review December 1, 2024 21:18
@Yard1
Copy link
Author

Yard1 commented Dec 1, 2024

@gaborbernat This is ready for review now. Happy to iterate on the API if needed (especially the Wrapper part, I think it's a bit clunky, though perhaps all it needs is just a rename).

@gaborbernat
Copy link
Member

The CI is failing.

@Yard1
Copy link
Author

Yard1 commented Dec 2, 2024

Thanks, will fix the type issues. Aside from that, I see the Windows tests are failing with

FAIL Required test coverage of 76.0% not reached. Total coverage: 66.65%

Considering there is no Windows implementation here, can you advise on how to deal with this?

@gaborbernat
Copy link
Member

gaborbernat commented Dec 2, 2024

I see a lot of markers to ignore typing errors. This is not the right approach. We should make the typing correct, not ignore the errors generated.

This is the same for documentation, links that you nitpick ignored.

@Yard1
Copy link
Author

Yard1 commented Dec 2, 2024

Thanks - I'm still iterating on this. Some of the ignores are copied/follow patterns as in existing code. Will mark as draft until I am done.

@Yard1 Yard1 marked this pull request as draft December 2, 2024 23:40
@Yard1
Copy link
Author

Yard1 commented Dec 3, 2024

In general the current way the async APIs are implemented necessitates ignores. I would like to avoid breaking changes, but it seems quite challenging to fix that otherwise (and I wouldn't want to do this in this PR anyway).

@Yard1
Copy link
Author

Yard1 commented Dec 3, 2024

I think I managed to get rid of most of the added type ignores (and the sphinx nitpick ignores). The ones that remain + linter noqas should be consistent with the rest of the codebase.

@Yard1 Yard1 marked this pull request as ready for review December 3, 2024 01:05
@gaborbernat
Copy link
Member

I think I managed to get rid of most of the added type ignores (and the sphinx nitpick ignores). The ones that remain + linter noqas should be consistent with the rest of the codebase.

Thank you, heads up that I will be unavailable until the new year, so I will not be able to accept this change request until then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request add RwLock
2 participants