Multi-threaded code checks can be skipped by declaring `@NotThreadSafe` #3399

stevenschlansker · 2025-04-11T17:37:31Z

Fixes #3390

Before opening a 'pull request'

Search existing issues and pull requests to see if the issue was already discussed.
Check our discussions to see if the issue was already discussed.
Check for specific project we support to raise the issue on, under spotbugs
Do not open intellij plugin issues here, open them at intellij-plugin

Make sure these boxes are checked before submitting your PR -- thank you!

Added an entry into CHANGELOG.md if you have changed SpotBugs code

CHANGELOG.md

gtoison · 2025-04-13T08:20:24Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

@@ -57,6 +57,9 @@ private MultiThreadedCodeIdentifierUtils() {

    public static boolean isPartOfMultiThreadedCode(ClassContext classContext) {
        JavaClass javaClass = classContext.getJavaClass();
+        if (notThreadSafe(javaClass.getAnnotationEntries())) {
+            return false;
+        }


I think it would be better to introduce a new method here: now the name of the method does not match with what it does

Hi @gtoison , I am not sure I understand your request here. From my understanding, "multi-threaded code" here refers to code that is in use from multiple threads and should be checked.
The @NotThreadSafe annotation is an explicit declaration from the user - "this code is not intended to be used from multiple threads". Therefore, any code marked @NotThreadSafe is definitionally not "part of multi-threaded code", as the user has told us it is not.

Do you have a better method name to propose?

@JuditKnoll previously indicated this would be a good place for the logic to live - perhaps they have a suggestion of the best way to resolve this

Currently isPartOfMultiThreadedCode() means "that class does things that imply intent to use multiple threads".
On the other hand, @NotThreadSafe means "this won't work when used from multiple threads".

I think something like isNotThreadSafe() or isExplicitelyNotThreadSafe() would convey that meaning.

Currently this function is only used in the SharedVariableAtomicityDetector to decide whether the class should be visited and the bugs reported. To my best knowledge, there are no ongoing work (open PRs in SpotBugs) depending on this function.
From the detector's POV, it's true, it would prevent Fps, however there could be another usecase, where this annotation check is not necessary. (It's true that it could be easily refactored, if it's necessary, if the code depending on it is inside SpotBugs.)
Introducing a new method with the @NotThreadSafe annotation check and other functionalities of the function leaves open this option (of course with calling the new method from the SharedVariableAtomicityDetector).
Maybe isThreadSafeMultiThreadedClass or shouldBeThreadSafeMultiThreadedClass names could reflect this check as well?

Fixes spotbugs#3390

JuditKnoll · 2025-04-16T10:09:18Z

spotbugsTestCases/src/fakeAnnotations/javax/annotation/concurrent/NotThreadSafe.java

+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+@Retention(RetentionPolicy.RUNTIME)


As far as I can see, the original NotThreadSafe annotation (see javadoc of the jakarta version or a copy of the javax annotation) has some different annotations: @Documented, @Target(TYPE) and @Retention(Class).

If there is no particular reason to change the fake annotations, it's best to keep them the same as the original ones IMO.

Ok I updated this

JuditKnoll · 2025-04-16T11:49:18Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

@@ -57,6 +57,9 @@ private MultiThreadedCodeIdentifierUtils() {

    public static boolean isPartOfMultiThreadedCode(ClassContext classContext) {
        JavaClass javaClass = classContext.getJavaClass();
+        if (notThreadSafe(javaClass.getAnnotationEntries())) {
+            return false;
+        }


Currently this function is only used in the SharedVariableAtomicityDetector to decide whether the class should be visited and the bugs reported. To my best knowledge, there are no ongoing work (open PRs in SpotBugs) depending on this function.
From the detector's POV, it's true, it would prevent Fps, however there could be another usecase, where this annotation check is not necessary. (It's true that it could be easily refactored, if it's necessary, if the code depending on it is inside SpotBugs.)
Introducing a new method with the @NotThreadSafe annotation check and other functionalities of the function leaves open this option (of course with calling the new method from the SharedVariableAtomicityDetector).
Maybe isThreadSafeMultiThreadedClass or shouldBeThreadSafeMultiThreadedClass names could reflect this check as well?

gtoison · 2025-04-18T13:29:06Z

I've pushed a commit to illustrate what I meant, IMO the isPartOfMultiThreadedCode() logic is complicated enough already and we should avoid adding a somewhat contradictory case in there.
IMO it is slightly better to break things down a bit, let me know if you think there's a better way

Revision of spotbugs#3399

stevenschlansker · 2025-04-22T16:31:43Z

Sounds good to me. Let me know if I should rebase / squash the commit or anything.

hgschmie · 2025-04-22T18:29:27Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

@@ -151,4 +151,18 @@ public static boolean isLocked(Method currentMethod, CFG currentCFG, LockDataflo
        }
        return false;
    }
+
+    /**
+     * @return <code>true</code> if the class is explicitely annotated with <code>NotThreadSafe</code> to document that it is not thread sage


Fixup on spotbugs#3399

gtoison

Thank you very much for the contribution!

gtoison · 2025-04-14T18:40:34Z

spotbugs/src/main/java/edu/umd/cs/findbugs/util/MultiThreadedCodeIdentifierUtils.java

@@ -57,6 +57,9 @@ private MultiThreadedCodeIdentifierUtils() {

    public static boolean isPartOfMultiThreadedCode(ClassContext classContext) {
        JavaClass javaClass = classContext.getJavaClass();
+        if (notThreadSafe(javaClass.getAnnotationEntries())) {
+            return false;
+        }


Currently isPartOfMultiThreadedCode() means "that class does things that imply intent to use multiple threads".
On the other hand, @NotThreadSafe means "this won't work when used from multiple threads".

I think something like isNotThreadSafe() or isExplicitelyNotThreadSafe() would convey that meaning.

stevenschlansker mentioned this pull request Apr 11, 2025

Mark classes / code as not threadsafe #3390

Closed

gtoison requested changes Apr 13, 2025

View reviewed changes

Multi-threaded code checks can be skipped by declaring @NotThreadSafe

7ad2b2b

Fixes spotbugs#3390

stevenschlansker force-pushed the not-multi-threaded branch from bc6f4ef to 7ad2b2b Compare April 14, 2025 18:00

JuditKnoll reviewed Apr 16, 2025

View reviewed changes

Separate the "has multi threading" and "not thread safe" concerns

cf84e1e

ThrawnCA approved these changes Apr 22, 2025

View reviewed changes

JuditKnoll approved these changes Apr 22, 2025

View reviewed changes

NotThreadSafe annotation declaration alignment

38034bd

Revision of spotbugs#3399

stevenschlansker requested a review from gtoison April 22, 2025 16:31

hgschmie reviewed Apr 22, 2025

View reviewed changes

NotThreadSafe typo fix

9ecff34

Fixup on spotbugs#3399

JuditKnoll approved these changes Apr 23, 2025

View reviewed changes

gtoison approved these changes Apr 23, 2025

View reviewed changes

JuditKnoll merged commit b584409 into spotbugs:master Apr 23, 2025
15 checks passed

JuditKnoll added this to the Spotbugs 4.9.4 milestone Apr 23, 2025

hazendaz self-assigned this Apr 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Multi-threaded code checks can be skipped by declaring `@NotThreadSafe` #3399

Multi-threaded code checks can be skipped by declaring `@NotThreadSafe` #3399

Uh oh!

stevenschlansker commented Apr 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

gtoison Apr 13, 2025

Uh oh!

stevenschlansker Apr 14, 2025

Uh oh!

stevenschlansker Apr 14, 2025

Uh oh!

gtoison Apr 14, 2025

Uh oh!

JuditKnoll Apr 16, 2025

Uh oh!

JuditKnoll Apr 16, 2025

Uh oh!

stevenschlansker Apr 22, 2025

Uh oh!

JuditKnoll Apr 16, 2025

Uh oh!

gtoison commented Apr 18, 2025

Uh oh!

stevenschlansker commented Apr 22, 2025

Uh oh!

hgschmie Apr 22, 2025

Uh oh!

stevenschlansker Apr 22, 2025

Uh oh!

gtoison left a comment

Uh oh!

gtoison Apr 14, 2025

Uh oh!

Uh oh!

Uh oh!

Multi-threaded code checks can be skipped by declaring @NotThreadSafe #3399

Multi-threaded code checks can be skipped by declaring @NotThreadSafe #3399

Uh oh!

Conversation

stevenschlansker commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

gtoison commented Apr 18, 2025

Uh oh!

stevenschlansker commented Apr 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gtoison left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Multi-threaded code checks can be skipped by declaring `@NotThreadSafe` #3399

Multi-threaded code checks can be skipped by declaring `@NotThreadSafe` #3399

stevenschlansker commented Apr 11, 2025 •

edited

Loading