Enables SharedVariableAtomicityDetector #6025

kevinrr888 · 2025-12-16T21:17:29Z

Enables the SharedVariableAtomicityDetector SpotBugs detector. Resolved all race conditions and all false-positives (with suppressions or NotThreadSafe annotation)

closes #5636

Enables the SharedVariableAtomicityDetector SpotBugs detector. Resolved all race conditions and all false-positives (with suppressions or NotThreadSafe annotation)

kevinrr888

Provided my reasoning in case it's helpful in review

kevinrr888 · 2025-12-16T21:18:48Z

core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java

+@NotThreadSafe
 class RFileScanner extends ScannerOptions implements Scanner {


Scanners are not thread safe objects

kevinrr888 · 2025-12-16T21:19:31Z

core/src/main/java/org/apache/accumulo/core/clientImpl/ScannerOptions.java

+@NotThreadSafe
 public class ScannerOptions implements ScannerBase {


Scanners are not thread safe

kevinrr888 · 2025-12-16T21:19:53Z

core/src/main/java/org/apache/accumulo/core/clientImpl/TabletServerBatchReaderIterator.java

+  @NotThreadSafe
  private class QueryTask implements Runnable {


QueryTask is not used as/expected to be a thread-safe object

kevinrr888 · 2025-12-16T21:20:34Z

core/src/main/java/org/apache/accumulo/core/clientImpl/ThriftTransportPool.java

+  @NotThreadSafe
  private static class CachedTTransport extends TTransport {


CachedTTransport is only modified by a single thread at a time (must be reserved()/unreserved())

kevinrr888 · 2025-12-16T21:21:07Z

core/src/main/java/org/apache/accumulo/core/crypto/streams/BlockedInputStream.java

+@NotThreadSafe
 public class BlockedInputStream extends InputStream {


BlockedInputStream is a wrapper for DataInputStream, which is not thread safe

kevinrr888 · 2025-12-16T21:36:45Z

server/tserver/src/main/java/org/apache/accumulo/tserver/NativeMap.java

+  @NotThreadSafe
  private static class NMSKVIter implements InterruptibleIterator {


Iterators are not thread safe

kevinrr888 · 2025-12-16T21:37:31Z

server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java

    private static final Logger log = LoggerFactory.getLogger(AssignmentWatcher.class);

-    private static long longAssignments = 0;
+    private static volatile long longAssignments = 0;


Set in a thread, but get in another thread, needs to be volatile

kevinrr888 · 2025-12-16T21:38:02Z

...tserver/src/test/java/org/apache/accumulo/tserver/tablet/CompactableImplFileManagerTest.java

+  @NotThreadSafe
  static class TestFileManager extends CompactableImpl.FileManager {


Test class used in single threaded test

kevinrr888 · 2025-12-16T21:38:16Z

test/src/main/java/org/apache/accumulo/test/functional/ErrorThrowingIterator.java

+@NotThreadSafe
 public class ErrorThrowingIterator extends WrappingIterator {


accumulo iterators are not thread safe

kevinrr888 · 2025-12-16T21:38:39Z

test/src/main/java/org/apache/accumulo/test/performance/scan/CollectTabletStats.java

+  @NotThreadSafe
  private abstract static class Test implements Runnable {


These are updated in this "Test" thread and checked in main thread, but they aren't checked until execution has completed, so fine not to use volatile or sync.

kevinrr888 · 2025-12-16T21:58:59Z

core/src/main/java/org/apache/accumulo/core/util/CountingInputStream.java

+@NotThreadSafe
 public final class CountingInputStream extends FilterInputStream {


This is a stream and the impl shows this is not intended to be thread safe

keith-turner

How does this static analysis pick which classes to inspect? Does every class that has non volatile and mutable fields have to be marked as not thread safe?

keith-turner · 2025-12-16T22:10:43Z

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLock.java

    QueueLock qlock;
    byte[] userData;
-    long entry = -1;
+    AtomicLong entry;


Changes may not be needed to this class. I think it is only used by a single thread, but not 100% sure. Did some sampling and found this usually created as a local var in a function to try to obtain a lock in zookeeper, but the local var is not shared among threads.

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLock.java

kevinrr888 · 2025-12-17T15:28:30Z

How does this static analysis pick which classes to inspect? Does every class that has non volatile and mutable fields have to be marked as not thread safe?

I believe it looks at every class, but not entirely sure how it chooses what to flag. I think it looks for common non-thread safe patterns and flags those (e.g., getting and then setting an atomic var in two operations, modifying a non-volatile/non-atomic var in a run(), but also having a getter in that same class, etc.). The documentation is pretty vague. https://spotbugs.readthedocs.io/en/latest/detectors.html

It does not seem that every class needs to be marked/considered, I only had to change ~40 for 2.1

…safe instead

kevinrr888 · 2025-12-17T19:01:26Z

Looked into what the merge for main would look like, thankfully very few changes needed. This is what I'll be fixing in the merge to main:

LoadFiles.Loader - not used as or expected to be thread safe
SystemInformation - is used as and expected to be thread safe - timestamp needs to be volatile

core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java

server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Scanner.java

keith-turner · 2025-12-20T00:22:47Z

server/base/src/main/java/org/apache/accumulo/server/tablets/TabletTime.java

-      if (this.nextTime.get() < time) {
-        this.nextTime.set(time);
-      }
+      this.nextTime.getAndUpdate(val -> Math.max(val, finalTime));


Maybe do not need finalTime.

Suggested change

this.nextTime.getAndUpdate(val -> Math.max(val, finalTime));

this.nextTime.getAndUpdate(val -> Math.max(val, time + 1));

Yeah, fixed in f827d1a. When looking at this again, realized this method is only called in the constructor of Tablet before TabletTime is available to multiple threads, so technically no changes were needed at all here and there was no bug. But only using one atomic op here is still probably better.

server/base/src/main/java/org/apache/accumulo/server/tablets/TabletTime.java

* changed `batchTimeout = scanner.getTimeout` to `batchTimeout = scanner.getBatchTimeout` in ClientSideIteratorScanner, appeared to be calling incorrect method * address review

dlmarion

Still reviewing....

core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java

…w PR

Enables SharedVariableAtomicityDetector

00db6e1

Enables the SharedVariableAtomicityDetector SpotBugs detector. Resolved all race conditions and all false-positives (with suppressions or NotThreadSafe annotation)

kevinrr888 added this to the 2.1.5 milestone Dec 16, 2025

kevinrr888 self-assigned this Dec 16, 2025

kevinrr888 commented Dec 16, 2025

View reviewed changes

kevinrr888 added 2 commits December 16, 2025 16:41

Merge branch '2.1' into 2.1-feature-5636

11a8056

NotThreadSafe to CountingInputStream

5784d49

kevinrr888 commented Dec 16, 2025

View reviewed changes

keith-turner reviewed Dec 16, 2025

View reviewed changes

kevinrr888 added 2 commits December 17, 2025 11:44

revert changes to DistributedReadWriteLock.ReadLock, mark not thread …

72d9e82

…safe instead

formatting

2d8ddd2

kevinrr888 linked an issue Dec 18, 2025 that may be closed by this pull request

Enable SharedVariableAtomicityDetector SpotBugs detector #5636

Open

keith-turner reviewed Dec 20, 2025

View reviewed changes

kevinrr888 added 2 commits December 22, 2025 11:18

changes:

f827d1a

* changed `batchTimeout = scanner.getTimeout` to `batchTimeout = scanner.getBatchTimeout` in ClientSideIteratorScanner, appeared to be calling incorrect method * address review

remove NotThreadSafe from Scanner, add suppression

2b3db90

dlmarion reviewed Dec 22, 2025

View reviewed changes

core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java Outdated Show resolved Hide resolved

revert a change to ClientSideIteratorScanner in favor of opening a ne…

ce5c4ac

…w PR

kevinrr888 mentioned this pull request Dec 22, 2025

fix an incorrect call in ClientSideIteratorScanner #6036

Merged

		@NotThreadSafe
		class RFileScanner extends ScannerOptions implements Scanner {

		@NotThreadSafe
		public class ScannerOptions implements ScannerBase {

		@NotThreadSafe
		private static class CachedTTransport extends TTransport {

		@NotThreadSafe
		public class BlockedInputStream extends InputStream {

		@NotThreadSafe
		private static class NMSKVIter implements InterruptibleIterator {

		@NotThreadSafe
		static class TestFileManager extends CompactableImpl.FileManager {

		@NotThreadSafe
		public class ErrorThrowingIterator extends WrappingIterator {

		@NotThreadSafe
		private abstract static class Test implements Runnable {

		@NotThreadSafe
		public final class CountingInputStream extends FilterInputStream {

	this.nextTime.getAndUpdate(val -> Math.max(val, finalTime));
	this.nextTime.getAndUpdate(val -> Math.max(val, time + 1));

Enables SharedVariableAtomicityDetector #6025

Are you sure you want to change the base?

Enables SharedVariableAtomicityDetector #6025

Uh oh!

Conversation

kevinrr888 commented Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinrr888 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinrr888 commented Dec 17, 2025

Uh oh!

kevinrr888 commented Dec 17, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dlmarion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevinrr888 commented Dec 16, 2025 •

edited

Loading