Issue2328

The documentation for `threading.Lock` says (emphasis added):

    A factory function that returns a new primitive lock object. Once a thread has acquired it, subsequent attempts to acquire it block, until it is released; *any thread may release it*.

This is in contrast with `threading.RLock` which specifically says that a "reentrant lock must be released by the thread that acquired it."

Jython uses the same implementation for both types of locks (Lock.java), and it obeys the semantics of `RLock` by requiring that the releasing thread be the acquiring thread, raising an exception if that's not the case. This can cause problems for applications that make advanced use of locks. 

Consider this example in Jython:

  $ ../jython/dist/bin/jython
  Jython 2.7rc2+ (default:5064a5c5b1a3, Apr 14 2015, 06:28:43)
  [Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_40
  Type "help", "copyright", "credits" or "license" for more information.
  >>> import threading
  >>> threading.Lock is threading.RLock
  True
  >>> lock = threading.Lock()
  >>> lock.acquire()
  True
  >>> t = threading.Thread(target=lock.release)
  >>> t.start(); t.join()
  >>> Exception in thread Thread-1:Traceback (most recent call last):
    File "//jython/dist/Lib/threading.py", line 222, in _Thread__bootstrap
      self.run()
    File "//jython/dist/Lib/threading.py", line 213, in run
      self._target(*self._args, **self._kwargs)
  RuntimeError: cannot release un-acquired lock
  >>> lock.locked()
  True
 
Compare to the CPython and PyPy behaviour:

  $ /opt/local/bin/python2.7
  Python 2.7.9 (default, Dec 13 2014, 15:13:49)
  [GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.56)] on darwin
  Type "help", "copyright", "credits" or "license" for more information.
  >>> import threading
  >>> threading.Lock is threading.RLock
  False
  >>> lock = threading.Lock()
  >>> lock.acquire()
  True
  >>> t = threading.Thread(target=lock.release)
  >>> t.start(); t.join()
  >>> lock.locked()
  False

One real-world example of this is ZODB, at least its test suite. Exceptions like the following are fairly common (fortunately releasing the lock is the last thing the thread was going to do, and then the lock gets destroyed anyway so it's not an immediate issue in this case---the failure has no consequences for the test):

  Exception in thread commit:Traceback (most recent call last):
  File "//jython/dist/Lib/threading.py", line 222, in _Thread__bootstrap
    self.run()
  File "//jython/dist/Lib/threading.py", line 213, in run
    self._target(*self._args, **self._kwargs)
  File "//ZODB/src/ZODB/tests/BasicStorage.py", line 343, in commit
    self._storage.tpc_finish(t, callback)
  File "//ZODB/src/ZODB/utils.py", line 293, in __call__
    return func(*args, **kw)
  File "//ZODB/src/ZODB/DemoStorage.py", line 349, in tpc_finish
    self.changes.tpc_finish(transaction, func)
  File "//ZODB/src/ZODB/utils.py", line 293, in __call__
    return func(*args, **kw)
  File "//ZODB/src/ZODB/MappingStorage.py", line 318, in tpc_finish
    self._commit_lock.release()
  RuntimeError: cannot release un-acquired lock

Makes sense. We can change the underlying implementation to use a Semaphore with count of 1, and that should provide the desired semantics. Should be an easy fix, assuming it doesn't break anything else out there.

If RLock and Lock are still sharing the same implementation though, wouldn't the Semaphore(1) approach then turn around and break RLock (which has to be acquirable and releasable an arbitrary number of times, albeit by the same thread)? 

I haven't studied the implementation that much, but I was imagining something like making Lock a base class (AbstractLock) with a flag set by its subclasses Lock and RLock controlling whether 'isHeldByCurrentThread' is checked on release or not. But that was just my first thought and I don't know that much about how Java classes get exposed to Python. Or maybe you also meant a separate implementation of Lock with Semaphore and using the current implementation as RLock?

Two completely different behaviors, so we will want to separate in this fix:

threading.RLock will (continue) to use java.util.concurrent.locks.ReentrantLock

threading.Lock will now use java.util.concurrent.Semaphore

My apologies for the misunderstanding. I agree that a Semaphore(1)-based approach for Lock should be compatible with the CPython semantics.

So I looked into this, and got pretty far, with all lock tests passing, including ones previously skipped. Patch attached (use-semaphore-for-lock.patch).

However the problem is that threading.Condition is built so that it can take Lock.newCondition, which means it needs a class that implements the Lock interface.

We can readily implement this with lower-level synchronization primitives provided by java.util.concurrent, most likely LockSupport, but it's unfortunately too late for RC3.

Target 2.7.1

The j.u.c.locks.ReentrantLock class is built upon the standard AbstractQueuedSynchronizer class. The current JavaDoc for that class (http://docs.oracle.com/javase/8/docs/api/java/util/concurrent/locks/AbstractQueuedSynchronizer.html) defines a complete /non-reentrant/ implementation of the j.u.c.locks.Lock interface, including Condition support. It's only a dozen or so lines and can be unlocked by any thread.

Perhaps that could form the basis for the threading.Lock instead of a Semaphore?

In the attached use-mutex-example-for-lock.patch, I tried the approach of using the Mutex example for Lock and sticking with ReentrantLock for RLock. There had to be a bit of abstraction introduced for Condition to work correctly. 

It's not the prettiest Java I've ever written, but at least as a proof-of-concept it seems to work. All the tests in test_threading and test_thread pass (along with as much of regrtest-unix as I can get through---my machine hangs out of files around test_httpservers and I haven't adjusted my ulimits yet). The only exception is the newly-enabled `test_notify` in test_threading.ConditionTests. Enabling that test was part of the earlier patch, but the code in Condition to implement `notify(n)` didn't make it in and I'm not 100% sure how to deal with overloaded methods yet so I didn't try to add it. 

(Also, the exposed name of RLock is coming out wrong right now and I don't yet know enough to fix it:

$ dist/bin/jython
imporJython 2.7rc2+ (default:5064a5c5b1a3+, Apr 14 2015, 15:38:48)
[Java HotSpot(TM) 64-Bit Server VM (Oracle Corporation)] on java1.8.0_40
Type "help", "copyright", "credits" or "license" for more information.
>>> import threading
>>> threading.RLock
<type 'org.python.modules._threading.RLock'>
>>> threading.Lock
<type '_threading.Lock'>
)

The second version of the patch cleans things up a little bit to remove the abstract class that was exposed to Python in the `mro()`. It also fixes the `test_notify` test. 

I tried adding RLock to CoreExposed.includes to fix the name, but that made things worse. Clearly I still have no idea what I'm doing when it comes to that :)

Jason, thanks for trying out the AbstractQueuedSynchronizer example code, and figuring out how to fit in with the approach I sketched out. This looks great, and given that the example is well tested in terms of AQS we should be able to get this into RC3 after all.

Also, the newest patch looks even better.

Don't worry about the Jython adaptation, I can readily make that happen. It's a bit of a mix of example code, my attempt, and your contributions, but if you haven't done so already, please sign https://www.python.org/psf/contrib/contrib-form/ so we can get this committed. Thanks again!

Thank you! The PSF agreement was one I don't think I'd signed yet---done.

Fixed as of https://hg.python.org/jython/rev/0e103b371f5a

With some more changes in the subsequent fix in https://hg.python.org/jython/rev/dd4327161042 - specifically this fixed the issue raised by msg9849 around repr - and made it more like CPython as well in terms of reflecting the state of the Condition, Lock, and RLock