From e79695ac62a1549b030760fadc259cede19e07a0 Mon Sep 17 00:00:00 2001 From: "G. Blake Meike" Date: Wed, 18 Dec 2019 13:55:06 -0800 Subject: [PATCH] CBL-496: Fix use of temp directory Change-Id: Icf3c2302bb20c1fb3ac2e34798e7109e395f4574 --- .../com/couchbase/lite/AbstractDatabase.java | 23 ++--- .../lite/AbstractDatabaseConfiguration.java | 95 +++++++------------ .../internal/AbstractExecutionService.java | 12 ++- .../java/com/couchbase/lite/PreInitTest.java | 15 ++- .../lite/internal/ExecutionServiceTest.kt | 85 +++++++++-------- 5 files changed, 111 insertions(+), 119 deletions(-) diff --git a/lib/src/shared/main/java/com/couchbase/lite/AbstractDatabase.java b/lib/src/shared/main/java/com/couchbase/lite/AbstractDatabase.java index 43483c060..8bca8acc6 100644 --- a/lib/src/shared/main/java/com/couchbase/lite/AbstractDatabase.java +++ b/lib/src/shared/main/java/com/couchbase/lite/AbstractDatabase.java @@ -17,6 +17,7 @@ // package com.couchbase.lite; +import android.support.annotation.GuardedBy; import android.support.annotation.NonNull; import android.support.annotation.Nullable; @@ -28,7 +29,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; -import java.util.Locale; import java.util.Map; import java.util.Set; import java.util.concurrent.Executor; @@ -160,8 +160,9 @@ protected static void copy( String toPath = getDatabasePath(new File(config.getDirectory()), name).getPath(); if (toPath.charAt(toPath.length() - 1) != File.separatorChar) { toPath += File.separator; } - // Set the temp directory based on Database Configuration: - config.setTempDir(); + // Setting the temp directory from the Database Configuration is deprecated and will go away: + + CouchbaseLiteInternal.setupDirectories(config.getRootDirectory()); try { C4Database.copyDb( @@ -204,9 +205,7 @@ public static void setLogLevel(@NonNull LogDomain domain, @NonNull LogLevel leve } private static File getDatabasePath(File dir, String name) { - name = name.replaceAll("/", ":"); - name = String.format(Locale.ENGLISH, "%s.%s", name, DB_EXTENSION); - return new File(dir, name); + return new File(dir, name.replaceAll("/", ":") + "." + DB_EXTENSION); } //--------------------------------------------- @@ -221,7 +220,7 @@ private static File getDatabasePath(File dir, String name) { @NonNull private final Object lock = new Object(); // Main database lock object for thread-safety - // guarded by 'lock' + @GuardedBy("lock") private final Set activeReplications; // Executor for purge and posting Database/Document changes. @@ -266,7 +265,7 @@ protected AbstractDatabase(@NonNull String name, @NonNull DatabaseConfiguration this.name = name; // Copy configuration - this.config = config.readonlyCopy(); + this.config = config.readOnlyCopy(); this.shellMode = false; this.postExecutor = CouchbaseLiteInternal.getExecutionService().getSerialExecutor(); @@ -276,8 +275,8 @@ protected AbstractDatabase(@NonNull String name, @NonNull DatabaseConfiguration // synchronized on 'lock' this.activeReplications = new HashSet<>(); - // Set the temp directory based on Database Configuration: - config.setTempDir(); + // Setting the temp directory from the Database Configuration is deprecated and will go away: + CouchbaseLiteInternal.setupDirectories(config.getRootDirectory()); // Open the database: open(); @@ -295,6 +294,8 @@ protected AbstractDatabase(@NonNull String name, @NonNull DatabaseConfiguration * Dictionary as an input of the predict() method of the PredictiveModel. */ protected AbstractDatabase(C4Database c4db) { + CouchbaseLiteInternal.requireInit("Cannot create database"); + this.c4db = c4db; this.config = null; this.shellMode = true; @@ -345,7 +346,7 @@ public long getCount() { * @return the READONLY copied config object */ @NonNull - public DatabaseConfiguration getConfig() { return config.readonlyCopy(); } + public DatabaseConfiguration getConfig() { return config.readOnlyCopy(); } /** * Gets an existing Document object with the given ID. If the document with the given ID doesn't diff --git a/lib/src/shared/main/java/com/couchbase/lite/AbstractDatabaseConfiguration.java b/lib/src/shared/main/java/com/couchbase/lite/AbstractDatabaseConfiguration.java index 1ce82d6d0..fd232bce8 100644 --- a/lib/src/shared/main/java/com/couchbase/lite/AbstractDatabaseConfiguration.java +++ b/lib/src/shared/main/java/com/couchbase/lite/AbstractDatabaseConfiguration.java @@ -17,40 +17,37 @@ package com.couchbase.lite; import android.support.annotation.NonNull; +import android.support.annotation.Nullable; import com.couchbase.lite.internal.CouchbaseLiteInternal; -import com.couchbase.lite.internal.core.C4Base; +import com.couchbase.lite.internal.utils.Preconditions; abstract class AbstractDatabaseConfiguration { - private static final String TEMP_DIR_NAME = "CouchbaseLiteTemp"; - - private static String tempDir; //--------------------------------------------- // member variables //--------------------------------------------- - private boolean readonly; - private boolean customDir; - private String directory; + private final boolean readOnly; + + private String rootDirectory; + private String dbDirectory; //--------------------------------------------- // Constructors //--------------------------------------------- - protected AbstractDatabaseConfiguration() { - this(false, CouchbaseLiteInternal.getDbDirectoryPath()); - } + protected AbstractDatabaseConfiguration() { this((String) null, false); } - protected AbstractDatabaseConfiguration(@NonNull AbstractDatabaseConfiguration config) { - this(config.customDir, config.directory); + protected AbstractDatabaseConfiguration(@NonNull AbstractDatabaseConfiguration config, boolean readOnly) { + this(config.rootDirectory, readOnly); } - private AbstractDatabaseConfiguration(boolean customDir, String directory) { - this.readonly = false; - this.customDir = customDir; - this.directory = directory; + private AbstractDatabaseConfiguration(@Nullable String dir, boolean readOnly) { + CouchbaseLiteInternal.requireInit("Cannot create database configuration"); + this.readOnly = readOnly; + setRootDirectory(dir); } //--------------------------------------------- @@ -58,71 +55,43 @@ private AbstractDatabaseConfiguration(boolean customDir, String directory) { //--------------------------------------------- /** - * Returns the path to the directory to store the database in. + * Returns the path to the directory that contains the database. + * If this path has not been set explicitly (see: setDirectory below), + * then it is the system default. If it has been set explicitly, then it also contains + * the temporary directory that Couchbase uses as a scratch pad. * - * @return the directory + * @return the database directory */ @NonNull - public String getDirectory() { - return directory; - } + public String getDirectory() { return dbDirectory; } //--------------------------------------------- // Protected level access //--------------------------------------------- - protected AbstractDatabaseConfiguration setDirectory(@NonNull String directory) { - if (directory == null) { throw new IllegalArgumentException("directory cannot be null."); } - if (readonly) { throw new IllegalStateException("DatabaseConfiguration is readonly mode."); } - this.directory = directory; - this.customDir = true; - return this; - } - protected abstract DatabaseConfiguration getDatabaseConfiguration(); - protected boolean isReadonly() { - return readonly; - } + protected AbstractDatabaseConfiguration setDirectory(@NonNull String dir) { + Preconditions.checkArgNotNull(dbDirectory, "directory"); + if (readOnly) { throw new IllegalStateException("DatabaseConfiguration is readonly mode."); } + + setRootDirectory(dir); - protected void setReadonly(boolean readonly) { - this.readonly = readonly; + return this; } + protected boolean isReadOnly() { return readOnly; } + //--------------------------------------------- // Package level access //--------------------------------------------- - /** - * Set the temp directory based on Database Configuration. - * The default temp directory is APP_CACHE_DIR/Couchbase/tmp. - * If a custom database directory is set, the temp directory will be - * CUSTOM_DATABASE_DIR/Couchbase/tmp. - */ - void setTempDir() { - synchronized (AbstractDatabaseConfiguration.class) { - final String tempDir = getTempDir(); - if ((tempDir == null) || (!tempDir.equals(AbstractDatabaseConfiguration.tempDir))) { - AbstractDatabaseConfiguration.tempDir = tempDir; - C4Base.setTempDir(AbstractDatabaseConfiguration.tempDir); - } - } - } + DatabaseConfiguration readOnlyCopy() { return new DatabaseConfiguration(getDatabaseConfiguration(), true); } - DatabaseConfiguration readonlyCopy() { - final DatabaseConfiguration config = new DatabaseConfiguration(getDatabaseConfiguration()); - config.setReadonly(true); - return config; - } + String getRootDirectory() { return rootDirectory; } - /** - * Returns the temp directory. The default temp directory is APP_CACHE_DIR/Couchbase/tmp. - * If a custom database directory is set, the temp directory will be - * CUSTOM_DATABASE_DIR/Couchbase/tmp. - */ - private String getTempDir() { - return (!customDir) - ? CouchbaseLiteInternal.getTmpDirectory(TEMP_DIR_NAME) - : CouchbaseLiteInternal.getTmpDirectory(directory, TEMP_DIR_NAME); + private void setRootDirectory(@Nullable String dir) { + dbDirectory = CouchbaseLiteInternal.makeDbPath(dir); + rootDirectory = dir; } } diff --git a/lib/src/shared/main/java/com/couchbase/lite/internal/AbstractExecutionService.java b/lib/src/shared/main/java/com/couchbase/lite/internal/AbstractExecutionService.java index 64989fcc3..240fae69d 100644 --- a/lib/src/shared/main/java/com/couchbase/lite/internal/AbstractExecutionService.java +++ b/lib/src/shared/main/java/com/couchbase/lite/internal/AbstractExecutionService.java @@ -286,7 +286,7 @@ private void executeTask(@NonNull InstrumentedTask newTask) { running++; } catch (RejectedExecutionException e) { - dumpExecutorState(e, newTask); + dumpExecutorState(newTask, e); throw e; } } @@ -296,7 +296,7 @@ private void executeTask(@NonNull InstrumentedTask newTask) { // This shouldn't happen. Checking `spaceAvailable` should guarantee that the // underlying executor always has resources when we attempt to execute something. - private void dumpExecutorState(@NonNull RejectedExecutionException ex, @Nullable InstrumentedTask current) { + private void dumpExecutorState(@Nullable InstrumentedTask current, @Nullable RejectedExecutionException ex) { if (throttled()) { return; } dumpServiceState(executor, "size: " + running, ex); @@ -499,5 +499,13 @@ protected AbstractExecutionService(@NonNull ThreadPoolExecutor baseExecutor) { @NonNull @Override public CloseableExecutor getConcurrentExecutor() { return concurrentExecutor; } + + + //--------------------------------------------- + // Package-private methods + //--------------------------------------------- + + @VisibleForTesting + void dumpExecutorState() { concurrentExecutor.dumpExecutorState(null, new RejectedExecutionException()); } } diff --git a/lib/src/shared/test/java/com/couchbase/lite/PreInitTest.java b/lib/src/shared/test/java/com/couchbase/lite/PreInitTest.java index 75769205b..aba198035 100644 --- a/lib/src/shared/test/java/com/couchbase/lite/PreInitTest.java +++ b/lib/src/shared/test/java/com/couchbase/lite/PreInitTest.java @@ -31,12 +31,19 @@ public void testCreateDatabaseBeforeInit() throws CouchbaseLiteException { } @Test(expected = IllegalStateException.class) - public void testGetTmpDirectoryBeforeInit() { - CouchbaseLiteInternal.getTmpDirectory("fail"); + public void testGetConsoleBeforeInit() { + new Log().getConsole(); } @Test(expected = IllegalStateException.class) - public void testGetDbDirectoryBeforeInit() { - CouchbaseLiteInternal.getDbDirectoryPath(); + public void testGetFileBeforeInit() { + new Log().getFile(); } + + @Test(expected = IllegalStateException.class) + public void testCreateDBConfigBeforeInit() { + new DatabaseConfiguration(); + } + + } diff --git a/lib/src/shared/test/java/com/couchbase/lite/internal/ExecutionServiceTest.kt b/lib/src/shared/test/java/com/couchbase/lite/internal/ExecutionServiceTest.kt index d835946ce..770e6c913 100644 --- a/lib/src/shared/test/java/com/couchbase/lite/internal/ExecutionServiceTest.kt +++ b/lib/src/shared/test/java/com/couchbase/lite/internal/ExecutionServiceTest.kt @@ -24,6 +24,7 @@ import org.junit.Assert.assertTrue import org.junit.Assert.fail import org.junit.Before import org.junit.Test +import java.lang.AssertionError import java.util.Stack import java.util.concurrent.ArrayBlockingQueue import java.util.concurrent.CountDownLatch @@ -138,54 +139,60 @@ class ExecutionServiceTest : PlatformBaseTest() { val executor = tinyService.serialExecutor - // block the executor: it has a single thread so no tasks can run - val blockLatch = CountDownLatch(1) - executor.execute { blockLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS) } - - val nTasks = 10 - val startLatch = CountDownLatch(1) - val firstStartedLatch = CountDownLatch(1) - val firstFinishedLatch = CountDownLatch(1) - val finishLatch = CountDownLatch(nTasks) - // put some tasks into the serial queue. - // the first of these should be scheduled (but not run) - for (i in 0 until nTasks - 1) { - executor.execute { - try { - firstStartedLatch.countDown() - startLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS) - } catch (ignore: InterruptedException) { - } finally { - firstFinishedLatch.countDown() - finishLatch.countDown() + try { + // block the executor: it has a single thread so no tasks can run + val blockLatch = CountDownLatch(1) + executor.execute { blockLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS) } + + val nTasks = 10 + val startLatch = CountDownLatch(1) + val firstStartedLatch = CountDownLatch(1) + val firstFinishedLatch = CountDownLatch(1) + val finishLatch = CountDownLatch(nTasks) + // put some tasks into the serial queue. + // the first of these should be scheduled (but not run) + for (i in 0 until nTasks - 1) { + executor.execute { + try { + firstStartedLatch.countDown() + startLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS) + } catch (ignore: InterruptedException) { + } finally { + firstFinishedLatch.countDown() + finishLatch.countDown() + } } } - } - // clear the block and wait for the first of nTasks to start running - blockLatch.countDown() - assertTrue(firstStartedLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS)) - // the first of the nTasks is now blocking the queue. + // clear the block and wait for the first of nTasks to start running + blockLatch.countDown() + assertTrue(firstStartedLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS)) + // the first of the nTasks is now blocking the queue. - // fill the executor - val swampLatch = CountDownLatch(1) - val clearSwampLatch = swamp(tinyExecutor, swampLatch) + // fill the executor + val swampLatch = CountDownLatch(1) + val clearSwampLatch = swamp(tinyExecutor, swampLatch) - startLatch.countDown() - assertTrue(firstFinishedLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS)) - // the executing task completes but fails to restart the queue: + startLatch.countDown() + assertTrue(firstFinishedLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS)) + // the executing task completes but fails to restart the queue: - swampLatch.countDown() - assertTrue(clearSwampLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS)) - // the swamp is now drained + swampLatch.countDown() + assertTrue(clearSwampLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS)) + // the swamp is now drained - // should be stalled. - assertFalse(finishLatch.await(1, TimeUnit.SECONDS)) - assertEquals(nTasks - 1L, finishLatch.count); + // should be stalled. + assertFalse(finishLatch.await(1, TimeUnit.SECONDS)) + assertEquals(nTasks - 1L, finishLatch.count); - executor.execute { finishLatch.countDown() } + executor.execute { finishLatch.countDown() } - assertTrue(finishLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS)) + assertTrue(finishLatch.await(TIMEOUT_SEC, TimeUnit.SECONDS)) + } + catch (e: AssertionError) { + tinyService.dumpExecutorState(); + throw e; + } } // A stopped serial executor throws on further attempts to schedule