diff --git a/atlasdb-commons/src/main/java/com/palantir/common/concurrent/InterruptibleFuture.java b/atlasdb-commons/src/main/java/com/palantir/common/concurrent/InterruptibleFuture.java index eb6b1ae08a2..65a17f2ed9d 100644 --- a/atlasdb-commons/src/main/java/com/palantir/common/concurrent/InterruptibleFuture.java +++ b/atlasdb-commons/src/main/java/com/palantir/common/concurrent/InterruptibleFuture.java @@ -53,16 +53,16 @@ private enum State { private final Lock lock = new ReentrantLock(false); private final Condition condition = lock.newCondition(); - @GuardedBy(value = "lock") + @GuardedBy("lock") private V returnValue; - @GuardedBy(value = "lock") + @GuardedBy("lock") private Throwable executionException; - @GuardedBy(value = "lock") + @GuardedBy("lock") private volatile CancellationException cancellationException; - @GuardedBy(value = "lock") + @GuardedBy("lock") private volatile State state = State.WAITING_TO_RUN; private final FutureTask futureTask = new FutureTask( @@ -113,7 +113,7 @@ public final boolean cancel(boolean mayInterruptIfRunning) { return futureTask.cancel(mayInterruptIfRunning); } - @SuppressWarnings("GuardedByChecker") + @GuardedBy("lock") protected void noteFinished() { state = State.COMPLETED; condition.signalAll(); @@ -157,7 +157,7 @@ public final V get(long timeout, TimeUnit unit) } } - @SuppressWarnings("GuardedByChecker") + @GuardedBy("lock") private V getReturnValue() throws ExecutionException, CancellationException { if (cancellationException != null) { throw Throwables.chain( @@ -173,15 +173,23 @@ private V getReturnValue() throws ExecutionException, CancellationException { * @return true if and only if the task was canceled before it ever executed */ @Override - @SuppressWarnings("GuardedByChecker") public final boolean isCancelled() { - return cancellationException != null; + lock.lock(); + try { + return cancellationException != null; + } finally { + lock.unlock(); + } } @Override - @SuppressWarnings("GuardedByChecker") public final boolean isDone() { - return state == State.COMPLETED; + lock.lock(); + try { + return state == State.COMPLETED; + } finally { + lock.unlock(); + } } @Override diff --git a/atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/timestamp/InDbTimestampBoundStore.java b/atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/timestamp/InDbTimestampBoundStore.java index de944682c7d..c20fd98c287 100644 --- a/atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/timestamp/InDbTimestampBoundStore.java +++ b/atlasdb-dbkvs/src/main/java/com/palantir/atlasdb/keyvalue/dbkvs/timestamp/InDbTimestampBoundStore.java @@ -62,8 +62,7 @@ protected String getInitializingClassName() { private final PhysicalBoundStoreStrategy physicalBoundStoreStrategy; private final InitializingWrapper wrapper = new InitializingWrapper(); - @GuardedBy("this") // lazy init to avoid db connections in constructors - private DBType dbType; + private volatile DBType dbType; @GuardedBy("this") private Long currentLimit = null; @@ -221,11 +220,17 @@ public synchronized void storeUpperLimit(final long limit) { }); } - @GuardedBy("this") private DBType getDbType(Connection connection) { - if (dbType == null) { - dbType = ConnectionDbTypes.getDbType(connection); + DBType type = this.dbType; // volatile read + if (type == null) { + synchronized (this) { + type = this.dbType; + if (type == null) { + type = ConnectionDbTypes.getDbType(connection); + this.dbType = type; + } + } } - return dbType; + return type; } } diff --git a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/LockWatchValueScopingCacheImpl.java b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/LockWatchValueScopingCacheImpl.java index 678221bfe85..b1cd23362bd 100644 --- a/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/LockWatchValueScopingCacheImpl.java +++ b/atlasdb-impl-shared/src/main/java/com/palantir/atlasdb/keyvalue/api/cache/LockWatchValueScopingCacheImpl.java @@ -198,15 +198,15 @@ private synchronized void processCommitUpdate(long startTimestamp) { /** * In order to maintain the necessary invariants, we need to do the following: *

- * 1. For each new event, we apply it to the cache. The effects of this application is described in - * {@link LockWatchValueScopingCache}. - * 2. For each transaction, we must ensure that we store a snapshot of the cache at the sequence corresponding - * to the transaction's start timestamp. Note that not every sequence will have a corresponding timestamp, so we - * don't bother storing a snapshot for those sequences. Also note that we know that each call here will only - * ever have new events, and that consecutive calls to this method will *always* have increasing sequences - * (without this last guarantee, we'd need to store snapshots for all sequences). - * 3. For each transaction, we must create a transaction scoped cache. We do this now as we have tighter guarantees - * around when the cache is created, and thus deleted. + * 1. For each new event, we apply it to the cache. The effects of this application is described in + * {@link LockWatchValueScopingCache}. + * 2. For each transaction, we must ensure that we store a snapshot of the cache at the sequence corresponding + * to the transaction's start timestamp. Note that not every sequence will have a corresponding timestamp, so we + * don't bother storing a snapshot for those sequences. Also note that we know that each call here will only + * ever have new events, and that consecutive calls to this method will *always* have increasing sequences + * (without this last guarantee, we'd need to store snapshots for all sequences). + * 3. For each transaction, we must create a transaction scoped cache. We do this now as we have tighter guarantees + * around when the cache is created, and thus deleted. */ private synchronized void updateStores(TransactionsLockWatchUpdate updateForTransactions) { Multimap reversedMap = createSequenceTimestampMultimap(updateForTransactions); @@ -246,7 +246,7 @@ private synchronized boolean isNewEvent(LockWatchEvent event) { private synchronized void assertNoSnapshotsMissing(Multimap reversedMap) { Set sequences = reversedMap.keySet(); - if (sequences.stream().map(snapshotStore::getSnapshotForSequence).anyMatch(Optional::isEmpty)) { + if (sequences.stream().map(this::getSnapshotForSequence).anyMatch(Optional::isEmpty)) { log.warn( "snapshots were not taken for all sequences; logging additional information", SafeArg.of("numSequences", sequences), @@ -260,6 +260,10 @@ private synchronized void assertNoSnapshotsMissing(Multimap getSnapshotForSequence(Sequence sequence) { + return snapshotStore.getSnapshotForSequence(sequence); + } + private synchronized void updateCurrentVersion(Optional maybeUpdateVersion) { maybeUpdateVersion .filter(this::shouldUpdateVersion) diff --git a/atlasdb-service/src/main/java/com/palantir/atlasdb/proto/fork/ForkedJsonFormat.java b/atlasdb-service/src/main/java/com/palantir/atlasdb/proto/fork/ForkedJsonFormat.java index e2c7fa0f65f..3c253abe968 100644 --- a/atlasdb-service/src/main/java/com/palantir/atlasdb/proto/fork/ForkedJsonFormat.java +++ b/atlasdb-service/src/main/java/com/palantir/atlasdb/proto/fork/ForkedJsonFormat.java @@ -1240,7 +1240,7 @@ private static String escapeText(String input) { break; default: // Check for other control characters - if (c >= 0x0000 && c <= 0x001F) { + if (c <= 0x001F) { appendEscapedUnicode(builder, c); } else if (Character.isHighSurrogate(c)) { // Encode the surrogate pair using 2 six-character sequence (\\uXXXX\\uXXXX) diff --git a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/schema/stream/StreamTest.java b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/schema/stream/StreamTest.java index f913d28468d..0007eab28ce 100644 --- a/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/schema/stream/StreamTest.java +++ b/atlasdb-tests-shared/src/test/java/com/palantir/atlasdb/schema/stream/StreamTest.java @@ -657,13 +657,15 @@ public void startFirstAndFail(Transaction tx, long streamId) { @Override public void startSecondAndFinish(Transaction tx, long streamId) { StreamTestStreamStore ss = StreamTestStreamStore.of(txManager, StreamTestTableFactory.of()); - ss.storeStreams(tx, ImmutableMap.of(streamId, new ByteArrayInputStream(new byte[1]))); + ss.storeStreams(tx, ImmutableMap.of(streamId, new ByteArrayInputStream(new byte[] {0x42}))); } }); Optional stream = getStream(streamId); assertThat(stream).isPresent(); - assertThat(stream.get()).isNotNull(); + try (InputStream inputStream = stream.get()) { + assertThat(inputStream).isNotNull().hasBinaryContent(new byte[] {0x42}); + } } @Test diff --git a/build.gradle b/build.gradle index 4df36444203..d79d538c4f2 100644 --- a/build.gradle +++ b/build.gradle @@ -9,7 +9,7 @@ buildscript { dependencies { classpath 'com.netflix.nebula:gradle-info-plugin:12.1.6' classpath 'com.netflix.nebula:nebula-publishing-plugin:20.3.0' - classpath 'com.palantir.baseline:gradle-baseline-java:4.188.0' + classpath 'com.palantir.baseline:gradle-baseline-java:5.18.0' classpath 'com.palantir.gradle.conjure:gradle-conjure:5.41.0' classpath 'com.palantir.gradle.consistentversions:gradle-consistent-versions:2.15.0' classpath 'com.palantir.gradle.docker:gradle-docker:0.32.0' diff --git a/commons-db/src/main/java/com/palantir/nexus/db/sql/SQLString.java b/commons-db/src/main/java/com/palantir/nexus/db/sql/SQLString.java index 3d051b2a7a9..250368b4b44 100644 --- a/commons-db/src/main/java/com/palantir/nexus/db/sql/SQLString.java +++ b/commons-db/src/main/java/com/palantir/nexus/db/sql/SQLString.java @@ -33,6 +33,7 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.regex.Pattern; +import javax.annotation.Nullable; import javax.annotation.concurrent.GuardedBy; import org.apache.commons.lang3.Validate; @@ -86,6 +87,7 @@ protected interface CallableCheckedException { * Runs the provided callable while holding the lock for the override caches. * Callers replacing the caches should hold this lock. */ + @Deprecated protected static T runWithCacheLock(CallableCheckedException callable) throws E { synchronized (cacheLock) { return callable.call(); @@ -165,13 +167,12 @@ public static boolean isQueryRegistered(String key) { * @param dbType Look for queries registered with this override first * @return a SQLString object representing the stored query */ - @SuppressWarnings("GuardedByChecker") static FinalSQLString getByKey(final String key, DBType dbType) { assert isValidKey(key) : "Keys only consist of word characters"; // $NON-NLS-1$ assert registeredValues.containsKey(key) || registeredValuesOverride.containsKey(key) : "Couldn't find SQLString key: " + key + ", dbtype " + dbType; // $NON-NLS-1$ //$NON-NLS-2$ - FinalSQLString cached = cachedKeyed.get(key); + FinalSQLString cached = getCachedSql(key); if (null != cached) { callbackOnUse.noteUse((SQLString) cached.delegate); return cached; @@ -192,6 +193,13 @@ static FinalSQLString getByKey(final String key, DBType dbType) { return valueForKey; } + @Nullable + @SuppressWarnings("GuardedBy") // we're only doing a volatile read of current cache + private static FinalSQLString getCachedSql(String key) { + ImmutableMap cache = cachedKeyed; // volatile read + return cache == null ? null : cache.get(key); + } + static FinalSQLString getByKey(String key, Connection connection) throws PalantirSqlException { DBType type = DBType.getTypeFromConnection(connection); return getByKey(key, type); @@ -209,7 +217,6 @@ public static boolean isValidKey(final String key) { * @param sql The string to be used in a query * @return a SQLString object representing the given SQL */ - @SuppressWarnings("GuardedByChecker") static FinalSQLString getUnregisteredQuery(String sql) { assert !isValidKey(sql) : "Unregistered Queries should not look like keys"; // $NON-NLS-1$ return new FinalSQLString(new SQLString(sql)); @@ -423,9 +430,10 @@ protected static ImmutableMap getCachedUnregistered() { @Deprecated protected static void setCachedUnregistered(ImmutableMap _cachedUnregistered) {} - @SuppressWarnings("GuardedByChecker") protected static ImmutableMap getCachedKeyed() { - return cachedKeyed; + synchronized (cacheLock) { + return cachedKeyed; + } } protected static void setCachedKeyed(ImmutableMap cachedKeyed) { diff --git a/lock-api/src/main/java/com/palantir/lock/client/LockRefreshingLockService.java b/lock-api/src/main/java/com/palantir/lock/client/LockRefreshingLockService.java index c5771e99dbe..68905636f13 100644 --- a/lock-api/src/main/java/com/palantir/lock/client/LockRefreshingLockService.java +++ b/lock-api/src/main/java/com/palantir/lock/client/LockRefreshingLockService.java @@ -164,7 +164,8 @@ public boolean unlockAndFreeze(HeldLocksToken token) { } @Override - @SuppressWarnings("checkstyle:NoFinalizer") // TODO (jkong): Can we safely remove this without breaking things? + @SuppressWarnings({"checkstyle:NoFinalizer", "Finalize"}) // TODO (jkong): Can we safely remove this without + // breaking things? protected void finalize() throws Throwable { super.finalize(); if (!isClosed) { diff --git a/lock-api/src/main/java/com/palantir/lock/client/LockRefreshingRemoteLockService.java b/lock-api/src/main/java/com/palantir/lock/client/LockRefreshingRemoteLockService.java index 986dc08e7ff..187511889bf 100644 --- a/lock-api/src/main/java/com/palantir/lock/client/LockRefreshingRemoteLockService.java +++ b/lock-api/src/main/java/com/palantir/lock/client/LockRefreshingRemoteLockService.java @@ -123,7 +123,8 @@ private void refreshLocks() { } @Override - @SuppressWarnings("checkstyle:NoFinalizer") // TODO (jkong): Can we safely remove this without breaking things? + @SuppressWarnings({"checkstyle:NoFinalizer", "Finalize"}) + // TODO (jkong): Can we safely remove this without breaking things? protected void finalize() throws Throwable { super.finalize(); if (!isClosed) { diff --git a/lock-impl/src/main/java/com/palantir/lock/logger/LockDescriptorMapper.java b/lock-impl/src/main/java/com/palantir/lock/logger/LockDescriptorMapper.java index e7dd3163c73..cca54e36738 100644 --- a/lock-impl/src/main/java/com/palantir/lock/logger/LockDescriptorMapper.java +++ b/lock-impl/src/main/java/com/palantir/lock/logger/LockDescriptorMapper.java @@ -40,8 +40,7 @@ ObfuscatedLockDescriptor getDescriptorMapping(LockDescriptor descriptor) { } /** - * - * @return map from mapping to real descriptor. + * Return a map from mapping to real descriptor. */ Map getReversedMapper() { return mapper.entrySet().stream().collect(Collectors.toMap(Map.Entry::getValue, Map.Entry::getKey));