add threading.RWLock

[84401114-8e59-4056-83cb-632106c0b648]

BPO	8800
Nosy	@jcea, @pitrou, @kristjanvalur, @tiran, @njsmith, @asvetlov, @ofek, @elarivie
Files	rwlock.patch Added-ShrdExclLock-to-threading-and-multiprocessing.patch sharablelock.patch Added-ShrdExclLock-to-threading-and-multiprocessing-2.patch rwlock.patch rwlock.patch rwlock-sbt.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-feature', 'library'] title = 'add threading.RWLock' updated_at = user = 'https://github.com/kristjanvalur' ``` bugs.python.org fields: ```python activity = actor = 'asvetlov' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'kristjan.jonsson' dependencies = [] files = ['17448', '27350', '27359', '27363', '27385', '27412', '27422'] hgrepos = [] issue_num = 8800 keywords = ['patch', 'needs review'] message_count = 63.0 messages = ['106350', '106372', '106373', '106376', '106377', '106386', '168077', '168149', '171567', '171568', '171599', '171600', '171626', '171639', '171653', '171659', '171667', '171669', '171674', '171693', '171695', '171696', '171697', '171698', '171699', '171700', '171703', '171708', '171709', '171710', '171713', '171714', '171716', '171717', '171718', '171721', '171780', '171781', '171782', '171783', '171785', '171786', '171788', '171789', '171790', '171792', '171793', '171877', '171883', '171891', '171914', '171915', '171930', '171975', '171979', '172064', '172071', '172074', '274765', '274795', '287745', '360026', '360031'] nosy_count = 15.0 nosy_names = ['jcea', 'pitrou', 'kristjan.jonsson', 'christian.heimes', 'jyasskin', 'njs', 'asvetlov', 'neologix', 'vrutsky', 'sbt', 'mklauber', 'Sebastian.Noack', 'dan.oreilly', 'Ofekmeister', 'elarivie'] pr_nums = [] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue8800' versions = [] ```

[pitrou]

I don't like the API. Why should I write "rwlock.acquire_read()" instead of "rwlock.reader.acquire()"? There should be one obvious way to do it.

The fact that "reader_lock" and "writer_lock" return a new object every time is a bit suboptimal, IMO.

Also, RWLockBase should be private (_RWLockBase).

Ah, and the patch needs docs :)

[pitrou]

By the way, the fact that acquire_read() is documented as "Acquire the lock in shared mode" and acquire_write() as "Acquire the lock in exclusive mode" hints that perhaps RWLock is not the right name ;)

[84401114-8e59-4056-83cb-632106c0b648]

Ok, here is a new patch. It takes into account various comments and suggestions: 1) The interface is java-like. a single RWLock instance only has attributes reader_lock and writer_lock. 2) Since the "owning" array needs only be process local, the same semantics can be used for multiprocessing. Multiprocessing version only does minimal patching of the threading classes, similarly to Barrier. There is implementation coupling between threading and multiprocessing but that is probably ok. 3) Test are there, except that the final tests in the multiprocessing, those that used to rely on data gathered in lists, are nerfed. Not sure how to gather data when multiprocessing 4) I also didn't get the "manager" tests to work. I don't know what managers are or what they are supposed to do, and why they use proxy objects. I attempted to create a proxy object in managers.py but it failed the tests. Also, I am unsure if it is sufficient to proxy only the RWLock object, or if the _ReaderLock and _WriterLock instances that are returned should be proxied too.

Documentation i stole from Java. The locking policy is the same as the RecursiveReaderWiterLock from java, i.e. readers wait for writers, but otherwise no preference. Recursion is provided for readers and writers, but not upgrade/downgrade.

Again, this is not intended to be final code, particularly I would like help and suggestions for the multiprocessing tests and the manager code.

Also, while I strongly prefer RWLock, or ReaderWriterLock, or something similar, basically because it fits with our already java-ized threading module and the terminology from everywhere else in the field today, I'm not going to be enormously difficult about it.

cheers!

[e26428b1-70cf-4e9f-ae3c-9ef0478633fb]

Attached is a new version of Kristjan's patch with support for managers. (A threading._RWLockCore object is proxied and wrapped in a local instance of a subclass of threading.RWLock.)

Also I made multiprocessing.RWLock.__init__() use multiprocessing.util.register_after_fork() to clear self._owners. Otherwise on Unix you can run into trouble if a forked processes starts a new thread whose id was in self._owners at the time of the fork.

I found that test_many_readers() and test_recursion() tended to fail when run with processes (on Windows). This is because starting a process on Windows is slow, particularly with a debug build. (Some of the buildbots running Windows in a VM can be crazily slow.) Each reader only held the lock for 0.02 secs which is much less than the time to start a process on Windows. This meant that it was easy to never have overlapping ownership, causing the tests to fail.

I fixed this by starting the readers while holding an exclusive, waiting for period, and then releasing the exclusive lock. This makes it possible to change

self.assertTrue(max(nlocked) > 1)

to

self.assertEqual(max(nlocked), N)

Choosing timeouts to keep the buildbots happy can be a pain:-(

[e26428b1-70cf-4e9f-ae3c-9ef0478633fb]

Fixed patch because I didn't test on Unix...

[84401114-8e59-4056-83cb-632106c0b648]

Thanks Richard. I wonder if the issues with the multiprocessing tests can be fixed by making use of Barriers?

One of the reasons I introduced Barriers into the lib originally was my alarm at seeing the various springklings of different _wait() calls in the unittests, particularly in the threading module (there are Condition variable tests there still that have race conditions, that are cunnrently hidden by liberal use of _wait())

Then I started to wonder if it were appropriate to use a Barrier in the Condition variable tests, particularly given that the former is implemented by way of the latter :)

[e26428b1-70cf-4e9f-ae3c-9ef0478633fb]

Kristjan: you seem to have attached socketserver.patch to the wrong issue.

[pitrou]

Le vendredi 05 octobre 2012 à 09:51 +0000, Kristján Valur Jónsson a écrit :

Then I started to wonder if it were appropriate to use a Barrier in the Condition variable tests, particularly given that the former is implemented by way of the latter :)

Indeed, the reason I wrote the lock tests that way is that I want the tests to be independent of the primitives under test. Otherwise things become very messy.

[73219d1f-a633-4d07-b832-359bf52673c8]

What is the status of the patch?

[84401114-8e59-4056-83cb-632106c0b648]

Seems to have fizzled out due to the intense amount of bikeshedding required.

[njsmith]

Side note, in case anyone else finds themselves looking for the MSDN magazine article referenced above: it appears to be in the June 2007 issue, titled "CONCURRENCY: Synchronization Primitives New To Windows Vista".

For the moment at least, this link seems to work: http://download.microsoft.com/download/3/A/7/3A7FA450-1F33-41F7-9E6D-3AA95B5A6AEA/MSDNMagazineJune2007en-us.chm

[650b1006-82fe-4cd5-88e5-35e799dac110]

Hi,

Sorry to wake up a 10 years old discussion

But I think that you might be interested in the following Python package that I created and maintain since few years now:

https://pypi.org/project/readerwriterlock/

1- It implements the three type of reader writer lock:

• Read Priority
• Write Priority
• Fair Priority

2- It matches the interface of python threading.Lock

More specifically: def acquire(self, blocking: bool = True, timeout: float = -1) -> bool: def release(self) -> None:

As you can see it supports the 'blocking' and the 'timeout' parameters) and it uses the same methods name

3- It supports context manager (enter, __exit__)

4- It is also possible (currently not well documented) to provide a lock factory when initializing a new reader writer lock to specify the internal lock mechanism to use (by default it uses threading.Lock).

    def __init__(self, lock_factory: Callable[[], Lockable] = lambda: threading.Lock()) -> None:

This hidden feature allows to offer the possibility to implement your own lock mechanism (using port, file on disk, etc, ...) and the reader writer lock will internally use it (This open the door for multiprocessing locking)

[asvetlov]

Thanks for sharing, your project looks viable!

[zklaus]

Is there still interest in having an RWLock in python itself? I personally think it would be an interesting and important addition.

It seems this issue opens with a patch and quickly dives into implementation questions, but perhaps it would be good to have a general consensus on whether or not this should be added?

[pitrou]

I do think a RWLock (or SELock, whetever the name :-)) would be useful.

@gpshead What do you think?

[pitrou]

That said, and for the record, I probably wouldn't review such a PR as I'm not active anymore.

[lrlunin]

What is an official status of this feature request? Would a sufficient solution/implementation added to cpython if there is any?

[pitrou]

@lrlunin There is no "official" status :-)

Would a sufficient solution/implementation added to cpython if there is any?

Well, it will have to come with unit tests and proper documentation. It will also need to be reviewed and accepted by a motivated core developer.

You may also start by asking on https://discuss.python.org/c/core-dev/23 to raise awareness and gather more opinions.

[gpshead]

Given that https://pypi.org/project/readerwriterlock/ exists and there hasn't been a high demand for this, I suggest not adding it to the standard library. We provide the basics, things like that build upon them.

I did skim over the rwlock-sbt.patch linked above (which appeared to be the most recent iteration of the 11y old proposals) and it isn't necessarily a bad thing, but a reader/writer shared/exclusive lock implementation is a complicated thing to maintain.

If we were to add one... I think we should really not be reinventing the wheel ourselves, in pure Python. It'd be good to wrap the underlying platform's implementation such as pthread_rwlock on posix and presumably something equivalent available on Windows. Those will perform better.

Similar to how we had a poorly performing pure python RLock implementation in the past until https://github.com/python/cpython/issues/47251 (bpo-3001) fixed it in ~3.2.

[rafalkrupinski]

We provide the basics, things like that build upon them.

How come toml is basics and RWLock is not?