Change GameLock API (should be no behavior change)

xwords4/android/app/src/main/java/org/eehouse/android/xw4/BoardDelegate.java

@@ -2769,7 +2769,7 @@ public class BoardDelegate extends DelegateBase
             summary = thread.getSummary();
             gi = thread.getGI();
         } else {
-            try ( GameLock lock = GameLock.getFor( rowID ).tryLockRO() ) {
+            try ( GameLock lock = GameLock.tryLockRO( rowID ) ) {
                 if ( null != lock ) {
                     summary = DBUtils.getSummary( activity, lock );
                     gi = new CurGameInfo( activity );

xwords4/android/app/src/main/java/org/eehouse/android/xw4/DBUtils.java

@@ -366,7 +366,7 @@ public class DBUtils {
     public static void addRematchInfo( Context context, long rowid, String btAddr,
                                        String phone, String relayID, String p2pAddr )
     {
-        try ( GameLock lock = GameLock.getFor( rowid ).tryLock() ) {
+        try ( GameLock lock = GameLock.tryLock(rowid) ) {
             if ( null != lock ) {
                 GameSummary summary = getSummary( context, lock );
                 if ( null != btAddr ) {
@@ -1057,7 +1057,7 @@ public class DBUtils {
 
             setCached( rowid, null ); // force reread
 
-            lock = GameLock.getFor( rowid ).tryLock();
+            lock = GameLock.tryLock( rowid );
             Assert.assertNotNull( lock );
             notifyListeners( rowid, GameChangeType.GAME_CREATED );
         }
@@ -1119,7 +1119,7 @@ public class DBUtils {
 
     public static void deleteGame( Context context, long rowid )
     {
-        try ( GameLock lock = GameLock.getFor( rowid ).lock( 300 ) ) {
+        try ( GameLock lock = GameLock.lock( rowid, 300 ) ) {
             if ( null != lock ) {
                 deleteGame( context, lock );
             } else {

xwords4/android/app/src/main/java/org/eehouse/android/xw4/DelegateBase.java

@@ -663,7 +663,7 @@ public class DelegateBase implements DlgClickNotify,
     public void onStatusClicked( long rowid )
     {
         Log.d( TAG, "onStatusClicked(%d)", rowid );
-        try ( GameLock lock = GameLock.getFor( rowid ).tryLockRO() ) {
+        try ( GameLock lock = GameLock.tryLockRO( rowid ) ) {
             if ( null != lock ) {
                 GamePtr gamePtr = GameUtils.loadMakeGame( getActivity(), lock );
                 if ( null != gamePtr ) {

xwords4/android/app/src/main/java/org/eehouse/android/xw4/GameConfigDelegate.java

@@ -539,7 +539,7 @@ public class GameConfigDelegate extends DelegateBase
             GameLock gameLock = null;
             XwJNI.GamePtr gamePtr = null;
             if ( null == m_jniThread ) {
-                gameLock = GameLock.getFor( m_rowid ).tryLockRO();
+                gameLock = GameLock.tryLockRO( m_rowid );
                 if ( null != gameLock ) {
                     gamePtr = GameUtils.loadMakeGame( m_activity, m_giOrig,
                                                       gameLock );
@@ -1234,8 +1234,7 @@ public class GameConfigDelegate extends DelegateBase
     {
         if ( !isFinishing() ) {
             GameLock gameLock = m_jniThread == null
-                ? GameLock.getFor( m_rowid ).tryLock()
+                ? GameLock.tryLock( m_rowid ) : m_jniThread.getLock();
-                : m_jniThread.getLock();
             GameUtils.applyChanges( m_activity, m_gi, m_car, m_disabMap,
                                     gameLock, forceNew );
             DBUtils.saveThumbnail( m_activity, gameLock, null ); // clear it

xwords4/android/app/src/main/java/org/eehouse/android/xw4/GameLock.java

@@ -34,15 +34,28 @@ import android.support.annotation.NonNull;
 // Implements read-locks and write-locks per game.  A read lock is
 // obtainable when other read locks are granted but not when a
 // write lock is.  Write-locks are exclusive.
-public class GameLock implements AutoCloseable {
+//
+// Let's try representing a lock with something serializable. Let's not have
+// one public object per game, but rather let lots of objects represent the
+// same state. That way a lock can be grabbed by one thread or object (think
+// GamesListDelegate) and held for as long as it takes the game that's opened
+// to be closed. During that time it can be shared among objects
+// (BoardDelegate and JNIThread, etc.) that know how to manage their
+// interactions. (Note that I'm not doing this now -- found a way around it --
+// but that the capability is still worth having, and so the change is going
+// in. Having getFor() be public, and there being GameLock instances out there
+// whose state was "unlocked", was just dumb.)
+//
+// So the class everybody sees (GameLock) is not stored. The rowid it
+// holds is a key to a private Hash of state.
+
+public class GameLock implements AutoCloseable, Serializable {
     private static final String TAG = GameLock.class.getSimpleName();
 
     private static final boolean DEBUG_LOCKS = false;
     // private static final long ASSERT_TIME = 2000;
     private static final long THROW_TIME = 1000;
     private long m_rowid;
-    private Stack<Owner> mOwners = new Stack<>();
-    private boolean mReadOnly;
 
     private static class Owner {
         Thread mThread;
@@ -62,26 +75,195 @@ public class GameLock implements AutoCloseable {
         }
     }
 
-    @Override
+    private static class GameLockState {
-    public String toString()
+        long mRowid;
-    {
+        private Stack<Owner> mOwners = new Stack<>();
-        return String.format("{this: %H; rowid: %d; count: %d; ro: %b}",
+        private boolean mReadOnly;
-                             this, m_rowid, mOwners.size(), mReadOnly);
+
+        GameLockState( long rowid ) { mRowid = rowid; }
+
+        // We grant a lock IFF:
+        // * Count is 0
+        // OR
+        // * existing locks are ReadOnly and this request is readOnly
+        // OR
+        // * the requesting thread already holds the lock (later...)
+        // // This could be written to allow multiple read locks.  Let's
+        // // see if not doing that causes problems.
+
+        private GameLock tryLockImpl( boolean readOnly )
+        {
+            GameLock result = null;
+            synchronized ( mOwners ) {
+                if ( DEBUG_LOCKS ) {
+                    Log.d( TAG, "%s.tryLockImpl(ro=%b)", this, readOnly );
+                }
+                // Thread thisThread = Thread.currentThread();
+                boolean grant = false;
+                if ( mOwners.empty() ) {
+                    grant = true;
+                } else if ( mReadOnly && readOnly ) {
+                    grant = true;
+                    // } else if ( thisThread == mOwnerThread ) {
+                    //     grant = true;
+                }
+
+                if ( grant ) {
+                    mOwners.push( new Owner() );
+                    mReadOnly = readOnly;
+                    result = new GameLock( mRowid );
+                }
+            }
+            return result;
+        }
+
+        private GameLock tryLockRO()
+        {
+            GameLock result = tryLockImpl( true );
+            logIfNull( result, "tryLockRO()" );
+            return result;
+        }
+
+        private GameLock lockImpl( long timeoutMS, boolean readOnly ) throws InterruptedException
+        {
+            long startMS = System.currentTimeMillis();
+            long endMS = startMS + timeoutMS;
+            synchronized ( mOwners ) {
+                while ( null == tryLockImpl( readOnly ) ) {
+                    long now = System.currentTimeMillis();
+                    if ( now >= endMS ) {
+                        throw new GameLockedException();
+                    }
+                    mOwners.wait( endMS - now );
+                }
+            }
+
+            if ( DEBUG_LOCKS ) {
+                long tookMS = System.currentTimeMillis() - startMS;
+                Log.d( TAG, "%s.lockImpl() returning after %d ms", this, tookMS );
+            }
+            return new GameLock(mRowid);
+        }
+
+        // Version that's allowed to return null -- if maxMillis > 0
+        private GameLock lock( long maxMillis ) throws GameLockedException
+        {
+            Assert.assertTrue( maxMillis <= THROW_TIME );
+            GameLock result = null;
+            try {
+                result = lockImpl( maxMillis, false );
+            } catch (InterruptedException ex) {
+                Log.d( TAG, "lock(): got %s", ex.getMessage() );
+            }
+            if ( DEBUG_LOCKS ) {
+                Log.d( TAG, "%s.lock(%d) => %s", this, maxMillis, result );
+            }
+            logIfNull( result, "lock(maxMillis=%d)", maxMillis );
+            return result;
+        }
+
+        private GameLock tryLock()
+        {
+            GameLock result = tryLockImpl( false );
+            logIfNull( result, "tryLock()" );
+            return result;
+        }
+
+        public GameLock lock() throws InterruptedException
+        {
+            if ( BuildConfig.DEBUG ) {
+                DbgUtils.assertOnUIThread( false );
+            }
+            GameLock result = lockImpl( Long.MAX_VALUE, false );
+            Assert.assertNotNull( result );
+            return result;
+        }
+
+        private GameLock lockRO( long maxMillis )
+        {
+            Assert.assertTrue( maxMillis <= THROW_TIME );
+            GameLock lock = null;
+            try {
+                lock = lockImpl( maxMillis, true );
+            } catch ( InterruptedException ex ) {
+            }
+
+            logIfNull( lock, "lockRO(maxMillis=%d)", maxMillis );
+            return lock;
+        }
+
+        private void unlock()
+        {
+            synchronized ( mOwners ) {
+                if ( DEBUG_LOCKS ) {
+                    Log.d( TAG, "%s.unlock()", this );
+                }
+                Thread oldThread = mOwners.pop().mThread;
+
+                // It's ok for different threads to hold the same RO lock
+                if ( !mReadOnly && oldThread != Thread.currentThread() ) {
+                    Log.e( TAG, "unlock(): unequal threads: %s => %s", oldThread,
+                           Thread.currentThread() );
+                    Assert.fail();
+                }
+
+                if ( mOwners.empty() ) {
+                    mOwners.notifyAll();
+                }
+                if ( DEBUG_LOCKS ) {
+                    Log.d( TAG, "%s.unlock() DONE", this );
+                }
+            }
+        }
+
+        private boolean canWrite()
+        {
+            boolean result = !mReadOnly; // && 1 == mLockCount[0];
+            if ( !result ) {
+                Log.w( TAG, "%s.canWrite(): => false", this );
+            }
+            return result;
+        }
+
+        private void setOwner( Owner owner )
+        {
+            synchronized ( mOwners ) {
+                mOwners.pop();
+                mOwners.push( owner );
+            }
+        }
+
+        @Override
+        public String toString()
+        {
+            return String.format("{this: %H; rowid: %d; count: %d; ro: %b}",
+                                 this, mRowid, mOwners.size(), mReadOnly);
+        }
+
+        private void logIfNull( GameLock result, String fmt, Object... args )
+        {
+            if ( DEBUG_LOCKS && null == result ) {
+                String func = new Formatter().format( fmt, args ).toString();
+                Log.d( TAG, "%s.%s => null", this, func );
+                Log.d( TAG, "Unable to lock; cur owner: %s; would-be owner: %s",
+                       mOwners.peek(), new Owner() );
+            }
+        }
     }
 
     public static class GameLockedException extends RuntimeException {}
 
-    private static Map<Long, WeakReference<GameLock>> sLockMap = new HashMap<>();
+    private static Map<Long, GameLockState> sLockMap = new HashMap<>();
-    public static GameLock getFor( long rowid )
+    private static GameLockState getFor( long rowid )
     {
-        GameLock result = null;
+        GameLockState result = null;
         synchronized ( sLockMap ) {
             if ( sLockMap.containsKey( rowid ) ) {
-                result = sLockMap.get( rowid ).get();
+                result = sLockMap.get( rowid );
             }
             if ( null == result ) {
-                result = new GameLock( rowid );
+                result = new GameLockState( rowid );
-                sLockMap.put( rowid, new WeakReference(result) );
+                sLockMap.put( rowid, result );
             }
         }
         return result;
@@ -89,138 +271,35 @@ public class GameLock implements AutoCloseable {
 
     private GameLock( long rowid ) { m_rowid = rowid; }
 
-    // We grant a lock IFF:
+    public static GameLock tryLock( long rowid )
-    // * Count is 0
-    // OR
-    // * existing locks are ReadOnly and this request is readOnly
-    // OR
-    // * the requesting thread already holds the lock (later...)
-    private GameLock tryLockImpl( boolean readOnly )
     {
-        GameLock result = null;
+        return getFor( rowid ).tryLock();
-        synchronized ( mOwners ) {
-            if ( DEBUG_LOCKS ) {
-                Log.d( TAG, "%s.tryLockImpl(ro=%b)", this, readOnly );
-            }
-            // Thread thisThread = Thread.currentThread();
-            boolean grant = false;
-            if ( mOwners.empty() ) {
-                grant = true;
-            } else if ( mReadOnly && readOnly ) {
-                grant = true;
-            // } else if ( thisThread == mOwnerThread ) {
-            //     grant = true;
-            }
-
-            if ( grant ) {
-                mOwners.push( new Owner() );
-                mReadOnly = readOnly;
-                result = this;
-            }
-        }
-        return result;
-    }
-    
-    // // This could be written to allow multiple read locks.  Let's
-    // // see if not doing that causes problems.
-    public GameLock tryLock()
-    {
-        GameLock result = tryLockImpl( false );
-        logIfNull( result, "tryLock()" );
-        return result;
     }
 
-    public GameLock tryLockRO()
+    public static GameLock tryLockRO(long rowid)
     {
-        GameLock result = tryLockImpl( true );
+        return getFor( rowid ).tryLockRO();
-        logIfNull( result, "tryLockRO()" );
-        return result;
-    }
-
-    private GameLock lockImpl( long timeoutMS, boolean readOnly ) throws InterruptedException
-    {
-        long startMS = System.currentTimeMillis();
-        long endMS = startMS + timeoutMS;
-        synchronized ( mOwners ) {
-            while ( null == tryLockImpl( readOnly ) ) {
-                long now = System.currentTimeMillis();
-                if ( now >= endMS ) {
-                    throw new GameLockedException();
-                }
-                mOwners.wait( endMS - now );
-            }
-        }
-
-        if ( DEBUG_LOCKS ) {
-            long tookMS = System.currentTimeMillis() - startMS;
-            Log.d( TAG, "%s.lockImpl() returning after %d ms", this, tookMS );
-        }
-        return this;
     }
 
     @NonNull
-    public GameLock lock() throws InterruptedException
+    public static GameLock lock(long rowid) throws InterruptedException
     {
-        if ( BuildConfig.DEBUG ) {
+        return getFor( rowid ).lock();
-            DbgUtils.assertOnUIThread( false );
-        }
-        GameLock result = lockImpl( Long.MAX_VALUE, false );
-        Assert.assertNotNull( result );
-        return result;
     }
 
-    // Version that's allowed to return null -- if maxMillis > 0
+    public static GameLock lock( long rowid, long maxMillis ) throws GameLockedException
-    public GameLock lock( long maxMillis ) throws GameLockedException
     {
-        Assert.assertTrue( maxMillis <= THROW_TIME );
+        return getFor( rowid ).lock( maxMillis );
-        GameLock result = null;
-        try {
-            result = lockImpl( maxMillis, false );
-        } catch (InterruptedException ex) {
-            Log.d( TAG, "lock(): got %s", ex.getMessage() );
-        }
-        if ( DEBUG_LOCKS ) {
-            Log.d( TAG, "%s.lock(%d) => %s", this, maxMillis, result );
-        }
-        logIfNull( result, "lock(maxMillis=%d)", maxMillis );
-        return result;
     }
 
-    public GameLock lockRO( long maxMillis )
+    public static GameLock lockRO( long rowid, long maxMillis )
     {
-        Assert.assertTrue( maxMillis <= THROW_TIME );
+        return getFor( rowid ).lockRO( maxMillis );
-        GameLock lock = null;
-        try {
-            lock = lockImpl( maxMillis, true );
-        } catch ( InterruptedException ex ) {
-        }
-
-        logIfNull( lock, "lockRO(maxMillis=%d)", maxMillis );
-        return lock;
     }
 
     public void unlock()
     {
-        synchronized ( mOwners ) {
+        getFor( m_rowid ).unlock();
-            if ( DEBUG_LOCKS ) {
-                Log.d( TAG, "%s.unlock()", this );
-            }
-            Thread oldThread = mOwners.pop().mThread;
-
-            // It's ok for different threads to hold the same RO lock
-            if ( !mReadOnly && oldThread != Thread.currentThread() ) {
-                Log.e( TAG, "unlock(): unequal threads: %s => %s", oldThread,
-                       Thread.currentThread() );
-                Assert.fail();
-            }
-            
-            if ( mOwners.empty() ) {
-                mOwners.notifyAll();
-            }
-            if ( DEBUG_LOCKS ) {
-                Log.d( TAG, "%s.unlock() DONE", this );
-            }
-        }
     }
 
     @Override
@@ -234,14 +313,6 @@ public class GameLock implements AutoCloseable {
         return m_rowid;
     }
 
-    private void setOwner( Owner owner )
-    {
-        synchronized ( mOwners ) {
-            mOwners.pop();
-            mOwners.push( owner );
-        }
-    }
-
     public interface LockProc {
         public void gotLock( GameLock lock );
     }
@@ -268,10 +339,9 @@ public class GameLock implements AutoCloseable {
                         } catch ( Exception ex) {}
                     } else {
                         try {
-                            lock = GameLock
+                            GameLockState state = getFor( rowid );
-                                .getFor( rowid )
+                            lock = state.lockImpl( maxMillis, false );
-                                .lockImpl( maxMillis, false );
+                            state.setOwner( owner );
-                            lock.setOwner( owner );
                         } catch ( GameLockedException | InterruptedException gle ) {}
                     }
 
@@ -289,20 +359,6 @@ public class GameLock implements AutoCloseable {
     // used only for asserts
     public boolean canWrite()
     {
-        boolean result = !mReadOnly; // && 1 == mLockCount[0];
+        return getFor( m_rowid ).canWrite();
-        if ( !result ) {
-            Log.w( TAG, "%s.canWrite(): => false", this );
-        }
-        return result;
-    }
-
-    private void logIfNull( GameLock result, String fmt, Object... args )
-    {
-        if ( DEBUG_LOCKS && null == result ) {
-            String func = new Formatter().format( fmt, args ).toString();
-            Log.d( TAG, "%s.%s => null", this, func );
-            Log.d( TAG, "Unable to lock; cur owner: %s; would-be owner: %s",
-                       mOwners.peek(), new Owner() );
-        }
     }
 }

xwords4/android/app/src/main/java/org/eehouse/android/xw4/GameUtils.java

@@ -93,7 +93,7 @@ public class GameUtils {
     public static byte[] savedGame( Context context, long rowid )
     {
         byte[] result = null;
-        try (GameLock lock = GameLock.getFor( rowid ).tryLockRO() ) {
+        try (GameLock lock = GameLock.tryLockRO( rowid ) ) {
             if ( null != lock ) {
                 result = savedGame( context, lock );
             }
@@ -147,7 +147,7 @@ public class GameUtils {
         if ( null == lockDest ) {
             long groupID = DBUtils.getGroupForGame( context, lockSrc.getRowid() );
             long rowid = saveNewGame( context, gamePtr, gi, groupID );
-            lockDest = GameLock.getFor( rowid ).tryLock();
+            lockDest = GameLock.tryLock( rowid );
         } else {
             saveGame( context, gamePtr, gi, lockDest, true );
         }
@@ -160,7 +160,7 @@ public class GameUtils {
     public static boolean resetGame( Context context, long rowidIn )
     {
         boolean success = false;
-        try ( GameLock lock = GameLock.getFor( rowidIn ).lock( 500 ) ) {
+        try ( GameLock lock = GameLock.lock( rowidIn, 500 ) ) {
             if ( null != lock ) {
                 tellDied( context, lock, true );
                 resetGame( context, lock, lock, false );
@@ -225,7 +225,7 @@ public class GameUtils {
             lock = thread.getLock();
         } else {
             try {
-                lock = GameLock.getFor( rowid ).lockRO( maxMillis );
+                lock = GameLock.lockRO( rowid, maxMillis );
             } catch ( GameLock.GameLockedException gle ) {
                 Log.ex( TAG, gle );
             }
@@ -256,7 +256,7 @@ public class GameUtils {
         if ( null != thread ) {
             lockSrc = thread.getLock();
         } else {
-            lockSrc = GameLock.getFor( rowidIn ).lockRO( 300 );
+            lockSrc = GameLock.lockRO( rowidIn, 300 );
         }
 
         if ( null != lockSrc ) {
@@ -292,7 +292,7 @@ public class GameUtils {
     {
         boolean success;
         // does this need to be synchronized?
-        try ( GameLock lock = GameLock.getFor( rowid ).tryLock() ) {
+        try ( GameLock lock = GameLock.tryLock( rowid ) ) {
             if ( null != lock ) {
                 deleteGame( context, lock, informNow );
                 success = true;
@@ -397,7 +397,7 @@ public class GameUtils {
     public static Bitmap loadMakeBitmap( Context context, long rowid )
     {
         Bitmap thumb = null;
-        try ( GameLock lock = GameLock.getFor( rowid ).tryLockRO() ) {
+        try ( GameLock lock = GameLock.tryLockRO( rowid ) ) {
             if ( null != lock ) {
                 CurGameInfo gi = new CurGameInfo( context );
                 GamePtr gamePtr = loadMakeGame( context, gi, lock );
@@ -649,7 +649,7 @@ public class GameUtils {
         if ( DBUtils.ROWID_NOTFOUND != rowid ) {
             // Use tryLock in case we're on UI thread. It's guaranteed to
             // succeed because we just created the rowid.
-            try ( GameLock lock = GameLock.getFor( rowid ).tryLock() ) {
+            try ( GameLock lock = GameLock.tryLock( rowid ) ) {
                 Assert.assertNotNull( lock );
                 applyChanges( context, sink, gi, util, addr, null, lock, false );
             }
@@ -910,7 +910,7 @@ public class GameUtils {
             // have the lock and we'll never get it.  Better to drop
             // the message than fire the hung-lock assert.  Messages
             // belong in local pre-delivery storage anyway.
-            try ( GameLock lock = GameLock.getFor( rowid ).lock( 150 ) ) {
+            try ( GameLock lock = GameLock.lock( rowid, 150 ) ) {
                 if ( null != lock ) {
                     CurGameInfo gi = new CurGameInfo( context );
                     FeedUtilsImpl feedImpl = new FeedUtilsImpl( context, rowid );
@@ -973,7 +973,7 @@ public class GameUtils {
                                         String oldDict, String newDict )
     {
         boolean success;
-        try ( GameLock lock = GameLock.getFor( rowid ).lock(300) ) {
+        try ( GameLock lock = GameLock.lock( rowid, 300 ) ) {
             success = null != lock;
             if ( success ) {
                 byte[] stream = savedGame( context, lock );
@@ -1258,7 +1258,7 @@ public class GameUtils {
                     }
                 }
 
-                try ( GameLock lock = GameLock.getFor( rowid ).tryLockRO() ) {
+                try ( GameLock lock = GameLock.tryLockRO( rowid ) ) {
                     if ( null != lock ) {
                         CurGameInfo gi = new CurGameInfo( m_context );
                         MultiMsgSink sink = new MultiMsgSink( m_context, rowid );

xwords4/android/app/src/main/java/org/eehouse/android/xw4/XWServiceHelper.java

@@ -134,7 +134,7 @@ abstract class XWServiceHelper {
                 for ( int ii = 0; success && ii < rowids.length; ++ii ) {
                     long rowid = rowids[ii];
                     CurGameInfo gi = null;
-                    try ( GameLock lock = GameLock.getFor( rowid ).tryLockRO() ) {
+                    try ( GameLock lock = GameLock.tryLockRO( rowid ) ) {
                         // drop invite if can't open game; likely a dupe!
                         if ( null != lock ) {
                             gi = new CurGameInfo( mService );