Merge pull request #1568 from couchbase/issue/1563a

Fixed #1563 - Database deletion failing with 1.4-25

Author: pasin

src/main/java/com/couchbase/lite/Database.java

@@ -394,23 +394,30 @@ public Replication createPushReplication(URL remote) {
      * Deletes the database.
      */
     @InterfaceAudience.Public
-    public void delete() throws CouchbaseLiteException {
-        if (open.get()) {
-            if (!close()) {
-                throw new CouchbaseLiteException("The database was open, and could not be closed",
-                        Status.INTERNAL_SERVER_ERROR);
+    public synchronized void delete() throws CouchbaseLiteException {
+        // NOTE: synchronized Manager.lockDatabases to prevent Manager to give the deleting Database
+        //       instance. See also `Manager.getDatabase(String, boolean)`.
+        synchronized (manager.lockDatabases) {
+            Log.v(TAG, "Deleting %s", this);
+
+            if (open.get()) {
+                if (!close())
+                    throw new CouchbaseLiteException("The database was open, and could not be closed",
+                            Status.INTERNAL_SERVER_ERROR);
             }
-        }
-        manager.forgetDatabase(this);
-        if (!exists()) {
-            return;
-        }
 
-        File dir = new File(path);
-        if (!FileDirUtils.deleteRecursive(dir))
-            throw new CouchbaseLiteException("Was not able to delete the database directory",
-                    Status.INTERNAL_SERVER_ERROR);
+            manager.forgetDatabase(this);
+
+            if (!exists())
+                return;
 
+            File dir = new File(path);
+            if (!FileDirUtils.deleteRecursive(dir))
+                throw new CouchbaseLiteException("Was not able to delete the database directory",
+                        Status.INTERNAL_SERVER_ERROR);
+
+            Log.v(TAG, "Deleted %s", this);
+        }
     }
 
     /**
@@ -1232,7 +1239,7 @@ public List<Replication> getActiveReplications() {
     }
 
     @InterfaceAudience.Private
-    public boolean exists() {
+    public synchronized boolean exists() {
         return new File(path).exists();
     }
 
@@ -1268,7 +1275,7 @@ else if (storageType.equals(Manager.FORESTDB_STORAGE))
     }
 
     @InterfaceAudience.Private
-    public synchronized void open() throws CouchbaseLiteException {
+    public void open() throws CouchbaseLiteException {
         open(manager.getDefaultOptions(name));
     }
 
@@ -1450,73 +1457,78 @@ SymmetricKey createSymmetricKey(Object keyOrPassword) throws CouchbaseLiteExcept
     @InterfaceAudience.Public
     public synchronized boolean close() {
         storeRef.await();
-        try {
-            closing.set(true);
 
-            if (!open.get()) {
-                // Ensure that the database is forgotten:
-                manager.forgetDatabase(this);
-                return false;
-            }
+        // NOTE: synchronized Manager.lockDatabases to prevent Manager to give the deleting Database
+        //       instance. See also `Manager.getDatabase(String, boolean)`.
+        synchronized (manager.lockDatabases) {
+            try {
+                closing.set(true);
 
-            synchronized (databaseListeners) {
-                for (DatabaseListener listener : databaseListeners)
-                    listener.databaseClosing();
-            }
+                if (!open.get()) {
+                    // Ensure that the database is forgotten:
+                    manager.forgetDatabase(this);
+                    return false;
+                }
 
-            synchronized (lockViews) {
-                if (views != null) {
-                    for (View view : views.values())
-                        view.close();
+                synchronized (databaseListeners) {
+                    for (DatabaseListener listener : databaseListeners)
+                        listener.databaseClosing();
                 }
-                views = null;
-            }
 
-            // Make all replicators stop and wait:
-            boolean stopping = false;
-            synchronized (activeReplicators) {
-                for (Replication replicator : activeReplicators) {
-                    if (replicator.getStatus() == Replication.ReplicationStatus.REPLICATION_STOPPED)
-                        continue;
-                    replicator.stop();
-                    stopping = true;
+                synchronized (lockViews) {
+                    if (views != null) {
+                        for (View view : views.values())
+                            view.close();
+                    }
+                    views = null;
                 }
 
-                // maximum wait time per replicator is 60 sec.
-                // total maximum wait time for all replicators is between 60sec and 119 sec.
-                long timeout = Replication.DEFAULT_MAX_TIMEOUT_FOR_SHUTDOWN * 1000;
-                long startTime = System.currentTimeMillis();
-                while (activeReplicators.size() > 0 && stopping &&
-                        (System.currentTimeMillis() - startTime) < timeout) {
-                    try {
-                        activeReplicators.wait(timeout);
-                    } catch (InterruptedException e) {
+                // Make all replicators stop and wait:
+                boolean stopping = false;
+                synchronized (activeReplicators) {
+                    for (Replication replicator : activeReplicators) {
+                        if (replicator.getStatus() == Replication.ReplicationStatus.REPLICATION_STOPPED)
+                            continue;
+                        replicator.stop();
+                        stopping = true;
+                    }
+
+                    // maximum wait time per replicator is 60 sec.
+                    // total maximum wait time for all replicators is between 60sec and 119 sec.
+                    long timeout = Replication.DEFAULT_MAX_TIMEOUT_FOR_SHUTDOWN * 1000;
+                    long startTime = System.currentTimeMillis();
+                    while (activeReplicators.size() > 0 && stopping &&
+                            (System.currentTimeMillis() - startTime) < timeout) {
+                        try {
+                            activeReplicators.wait(timeout);
+                        } catch (InterruptedException e) {
+                        }
                     }
+                    // clear active replicators:
+                    activeReplicators.clear();
                 }
-                // clear active replicators:
-                activeReplicators.clear();
-            }
 
-            // Clear all replicators:
-            allReplicators.clear();
+                // Clear all replicators:
+                allReplicators.clear();
 
-            // cancel purge timer
-            cancelPurgeTimer();
+                // cancel purge timer
+                cancelPurgeTimer();
 
-            // Close Store:
-            if (store != null)
-                store.close();
+                // Close Store:
+                if (store != null)
+                    store.close();
 
-            // Clear document cache:
-            clearDocumentCache();
+                // Clear document cache:
+                clearDocumentCache();
 
-            // Forget database:
-            manager.forgetDatabase(this);
+                // Forget database:
+                manager.forgetDatabase(this);
 
-            open.set(false);
-            return true;
-        } finally {
-            closing.set(false);
+                open.set(false);
+                return true;
+            } finally {
+                closing.set(false);
+            }
         }
     }
 

src/main/java/com/couchbase/lite/Manager.java

@@ -77,6 +77,7 @@ public final class Manager {
 
     private ManagerOptions options;
     private File directoryFile;
+    /*package*/ Object lockDatabases = new Object();
     private Map<String, Database> databases;
     private Map<String, Object> encryptionKeys;
     private List<Replication> replications;
@@ -262,24 +263,26 @@ public File getDirectory() {
      */
     @InterfaceAudience.Public
     public void close() {
-        Log.d(Database.TAG, "Closing " + this);
-        // Close all database:
-        // Snapshot of the current open database to avoid concurrent modification as
-        // the database will be forgotten (removed from the databases map) when it is closed:
-        Database[] openDbs = databases.values().toArray(new Database[databases.size()]);
-        for (Database database : openDbs) {
-            database.close();
-        }
-        databases.clear();
+        synchronized (lockDatabases) {
+            Log.d(Database.TAG, "Closing " + this);
+
+            // Close all database:
+            // Snapshot of the current open database to avoid concurrent modification as
+            // the database will be forgotten (removed from the databases map) when it is closed:
+            Database[] openDbs = databases.values().toArray(new Database[databases.size()]);
+            for (Database database : openDbs)
+                database.close();
+            databases.clear();
 
-        // Stop reachability:
-        context.getNetworkReachabilityManager().stopListening();
+            // Stop reachability:
+            context.getNetworkReachabilityManager().stopListening();
 
-        // Shutdown ScheduledExecutorService:
-        if (workExecutor != null && !workExecutor.isShutdown()) {
-            Utils.shutdownAndAwaitTermination(workExecutor);
+            // Shutdown ScheduledExecutorService:
+            if (workExecutor != null && !workExecutor.isShutdown())
+                Utils.shutdownAndAwaitTermination(workExecutor);
+
+            Log.d(Database.TAG, "Closed " + this);
         }
-        Log.d(Database.TAG, "Closed " + this);
     }
 
     /**
@@ -512,7 +515,9 @@ public void setDefaultHttpClientFactory(HttpClientFactory defaultHttpClientFacto
      */
     @InterfaceAudience.Private
     public Collection<Database> allOpenDatabases() {
-        return databases.values();
+        synchronized (lockDatabases) {
+            return databases.values();
+        }
     }
 
     /**
@@ -551,25 +556,28 @@ public void run() {
      * @exclude
      */
     @InterfaceAudience.Private
-    public synchronized Database getDatabase(String name, boolean mustExist) {
-        if (options.isReadOnly())
-            mustExist = true;
-        Database db = databases.get(name);
-        if (db == null) {
-            if (!isValidDatabaseName(name))
-                throw new IllegalArgumentException("Invalid database name: " + name);
-            String path = pathForDatabaseNamed(name);
-            if (path == null)
-                return null;
-            db = new Database(path, name, this, options.isReadOnly());
-            if (mustExist && !db.exists()) {
-                Log.i(Database.TAG, "mustExist is true and db (%s) does not exist", name);
-                return null;
+    public Database getDatabase(String name, boolean mustExist) {
+        synchronized (lockDatabases) {
+            if (options.isReadOnly())
+                mustExist = true;
+            Database db = databases.get(name);
+            if (db == null) {
+                if (!isValidDatabaseName(name))
+                    throw new IllegalArgumentException("Invalid database name: " + name);
+                String path = pathForDatabaseNamed(name);
+                if (path == null)
+                    return null;
+                db = new Database(path, name, this, options.isReadOnly());
+                if (mustExist && !db.exists()) {
+                    Log.i(Database.TAG, "mustExist is true and db (%s) does not exist", name);
+                    return null;
+                }
+                db.setName(name);
+                databases.put(name, db);
             }
-            db.setName(name);
-            databases.put(name, db);
+            Log.v(Log.TAG_DATABASE, "getDatabase() %s %s", this, db);
+            return db;
         }
-        return db;
     }
 
     /**
@@ -1003,21 +1011,27 @@ private static Map<String, Object> parseSourceOrTarget(Map<String, Object> prope
      */
     @InterfaceAudience.Private
     protected void forgetDatabase(Database db) {
-        // remove from cached list of dbs
-        databases.remove(db.getName());
-
-        // remove from list of replications
-        // TODO: should there be something that actually stops the replication(s) first?
-        Iterator<Replication> replicationIterator = this.replications.iterator();
-        while (replicationIterator.hasNext()) {
-            Replication replication = replicationIterator.next();
-            if (replication.getLocalDatabase().getName().equals(db.getName())) {
-                replicationIterator.remove();
+        synchronized (lockDatabases) {
+            Log.v(Log.TAG_DATABASE, "Fogetting forgetDatabase() %s %s", this, db);
+
+            // remove from cached list of dbs
+            databases.remove(db.getName());
+
+            // remove from list of replications
+            // TODO: should there be something that actually stops the replication(s) first?
+            Iterator<Replication> replicationIterator = this.replications.iterator();
+            while (replicationIterator.hasNext()) {
+                Replication replication = replicationIterator.next();
+                if (replication.getLocalDatabase().getName().equals(db.getName())) {
+                    replicationIterator.remove();
+                }
             }
-        }
 
-        // Remove registered encryption key if available:
-        encryptionKeys.remove(db.getName());
+            // Remove registered encryption key if available:
+            encryptionKeys.remove(db.getName());
+
+            Log.v(Log.TAG_DATABASE, "Forgot forgetDatabase() %s %s", this, db);
+        }
     }
 
     /**

src/main/java/com/couchbase/lite/replicator/ReplicationInternal.java

@@ -878,17 +878,7 @@ public void onCompletion(Response httpResponse, Object result, Throwable e) {
                         Map<String, Object> response = (Map<String, Object>) result;
                         body.put("_rev", response.get("rev"));
                         remoteCheckpoint = body;
-                        boolean isOpen = false;
-                        try {
-                            if (db != null) {
-                                db.open();
-                                isOpen = true;
-                            }
-                        } catch (CouchbaseLiteException ex) {
-                            Log.w(Log.TAG_SYNC, "%s: Cannot open the database", ex, this);
-                        }
-
-                        if (isOpen) {
+                        if (db != null && db.isOpen()) {
                             Log.d(Log.TAG_SYNC,
                                     "%s: saved remote checkpoint, updating local checkpoint. RemoteCheckpoint: %s",
                                     this, remoteCheckpoint);

src/main/java/com/couchbase/lite/router/Router.java

@@ -622,6 +622,7 @@ public void start() {
                 connection.getResponseBody() == null &&
                 connection.getHeaderField("Content-Type") == null &&
                 dontOverwriteBody == false) {
+                connection.getResHeader().add("Content-Type", CONTENT_TYPE_JSON);
                 connection.setResponseBody(new Body("{\"ok\":true}".getBytes()));
             }
 
@@ -1141,13 +1142,17 @@ public Status do_PUT_Database(Database _db, String _docID, String _attachmentNam
         return new Status(Status.CREATED);
     }
 
-    public Status do_DELETE_Database(Database _db, String _docID, String _attachmentName)
-            throws CouchbaseLiteException {
+    public Status do_DELETE_Database(Database _db, String _docID, String _attachmentName) {
         if (getQuery("rev") != null) {
             return new Status(Status.BAD_REQUEST);
             // CouchDB checks for this; probably meant to be a document deletion
         }
-        db.delete();
+        try {
+            db.delete();
+        } catch (CouchbaseLiteException e) {
+            Log.w(TAG, "Failed to delete database: do_DELETE_Database()", e);
+            return e.getCBLStatus();
+        }
         return new Status(Status.OK);
     }