Issue2336

I came across this while running the `zodbshootout` to get an idea of Jython's performance under ZODB with high-concurrency. There had been 4 threads running, although when the error occurred there was only a single thread running (a cleanup phase of the benchmark). That cleanup phase has a loop that calls `gc.collect` and one of those calls threw this:

    java.lang.NullPointerException
	at org.python.modules.gc$WeakReferenceGC.equals(gc.java:709)
	at java.util.HashMap.removeNode(HashMap.java:819)
	at java.util.HashMap.remove(HashMap.java:798)
	at java.util.HashSet.remove(HashSet.java:235)
	at org.python.modules.gc.syncCollect(gc.java:1859)
	at org.python.modules.gc.collect_intern(gc.java:1789)
	at org.python.modules.gc.collect(gc.java:1721)

ZODB (or more specifically, persistent) does use the `gc.monitorObject` API as well as `weakref.WeakValueDictionary`: the values in the dictionary are monitored because we need a more deterministic cleanup of them then standard Java GC semantics offer.

An

Another bug for Stefan to look at. Nothing to delay final, but we should fix in 2.7.1 so we can get our perf stats! Also this means we should finally work on performance :) Looking forward to it!

However, it would still be awesome to get these performance numbers, whether bad or good. Jason, can you change your call to gc.collect as follows, monkeypatching sys.modules as necessary:

from java.lang import System

System.gc()

then we could get those stats for the time being.

Jim, I did find the workarounds to make the benchmarks run. The results are part of the two comments starting at https://github.com/zopefoundation/persistent/pull/20#issuecomment-95341970 Keep in mind, this is comparing CPython using C-acceleration modules with the pure python implementation.

Same as I replied in 2337 applies here:
Could you provide a (preferably minimal) code-sample that produces the error? To fix it reliably, we will need to pin it in a unit-test.

I don't think this is reliably reproducible in an "organic" situation, because it depends on the behaviour of the Java garbage collector, with a reference getting cleared at just the wrong time.

However, I can reliably demonstrate the problem in this reduced example:

  >>> import gc
  >>> from java.lang import Class
  >>> WeakReferenceGC = Class.forName('org.python.modules.gc$WeakReferenceGC')
  # We have to actually construct the right type, the constructor is protected
  # and Jython doesn't expose that to us; we'd get a plain WeakReference
  # if we tried WeakReferenceGC()
  >>> con = WeakReferenceGC.getDeclaredConstructors()[0]
  >>> con.setAccessible(True)
  >>> x = object()
  >>> ref = con.newInstance(x)
  # It works to start with
  >>> ref == ref
  True
  >>> ref.get() is x
  True
  # Now clean up the referent
  >>> del x
  >>> while ref.get():
  ...     _ = gc.collect()
  >>> ref.get()
  # Boom!
  >>> ref == ref
  Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
  java.lang.NullPointerException
	at org.python.modules.gc$WeakReferenceGC.equals(gc.java:709)
        ... 

  java.lang.NullPointerException: java.lang.NullPointerException

This patch should be null-safe; it makes the example work at least:

diff -r 95d09ba14aa7 src/org/python/modules/gc.java
--- a/src/org/python/modules/gc.java	Wed Apr 15 18:34:17 2015 -0400
+++ b/src/org/python/modules/gc.java	Fri Apr 24 08:22:23 2015 -0500
@@ -706,8 +706,12 @@

         public boolean equals(Object ob) {
             if (ob instanceof WeakReferenceGC) {
-                return ((WeakReferenceGC) ob).get().equals(get())
-                    && ((WeakReferenceGC) ob).hashCode() == hashCode();
+                WeakReferenceGC otherRef = (WeakReferenceGC)ob;
+                Object mine = get();
+                Object theirs = otherRef.get();
+                return (mine == theirs //same object, or both null
+                        || (mine != null && mine.equals(theirs))) //at least one is not null
+                    && hashCode() == otherRef.hashCode();
             } else if (ob instanceof WeakrefGCCompareDummy) {
                 return ((WeakrefGCCompareDummy) ob).compare != null
                         && ((WeakrefGCCompareDummy) ob).compare.equals(get());

Looks good to me so far. If it passes tests, I would merge it in. (However currently the repo is frozen because of release-phase)

I finally merged this fix too and also added a unit-test for it.

This issue is now fixed as of commit changeset: 7706:bfa13b3a5553.