Skip to content

Add FindThreadStartInConstructor (Sonar S2693) - #1238

Merged
steve-aom-elliott merged 1 commit into
mainfrom
add-rspec-s2693-thread-start-in-constructor
Sep 15, 2026
Merged

steve-aom-elliott merged 1 commit into
mainfrom
add-rspec-s2693-thread-start-in-constructor

Conversation

@steve-aom-elliott

Copy link
Copy Markdown
Contributor

Adds a search recipe covering S2693 — Threads should not be started in constructors.

Starting a thread before construction completes lets the new thread observe a partially-initialised receiver. The problem is worse in a non-final class: the superclass constructor starts the thread before the subclass has run any of its own initialisation, guaranteeing the subclass' fields are unset.

Fits alongside FindVirtualThreadOpportunities in org.openrewrite.java.migrate.lang — both are "your thread management could be modernised" search recipes; S2693's remedy is to move the start() out of the construction path and delegate to an ExecutorService.

What it flags

Mirrors sonar-java's ThreadStartedInConstructorCheck: a Thread.start() invocation whose enclosing scope, in a non-final class, is one of:

  • A constructor body
  • An instance field initializer (Thread t = ...; t.start(); at field-init position)
  • An instance initializer block ({ ... t.start(); ... })

Detection walks getCursor().getPath() outward from the call site:

First parent Effect
J.Lambda not flagged (deferred execution)
J.Block with isStatic() not flagged (static initializer)
J.MethodDeclaration if constructor → check enclosing class; else → not flagged
J.NewClass with body not flagged (anonymous class is effectively final)
J.ClassDeclaration (first, before any method) in instance field init / init block → check class

The enclosing-class check treats records and enums as effectively final — they can't be extended in the way S2693 cares about.

MethodMatcher("java.lang.Thread start()", true) uses matchOverrides=true so myThread.start() on a Thread subclass instance fires too.

Tagged RSPEC-S2693 only. Sonar-java's rule metadata has no CWE for S2693 — it's a design-quality issue rather than a specific security weakness.

Known limitations (not flagged)

  • Runnable.run() / ExecutorService.submit() from a constructor — same "leaks this" risk, different rule surface.
  • start() called from a helper method invoked by the constructor — cross-method flow isn't tracked. Sonar-java has the same intra-method-only behaviour.
  • new Thread(this).start() isn't specifically distinguished from plain t.start() — sonar-java doesn't inspect the Runnable target either.

Tests

9 tests, all passing.

@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Sep 11, 2026
@steve-aom-elliott steve-aom-elliott moved this from In Progress to Ready to Review in OpenRewrite Sep 11, 2026
@steve-aom-elliott
steve-aom-elliott marked this pull request as ready for review September 11, 2026 21:16
Search recipe for the shape flagged by sonar-java
ThreadStartedInConstructorCheck (RSPEC-S2693): Thread.start()
reached during construction of a non-final class. The new thread
can observe a partially-initialised object, and any subclass' fields
are guaranteed unset because the superclass constructor starts the
thread before the subclass' own initialisation runs.

Fits alongside FindVirtualThreadOpportunities in
org.openrewrite.java.migrate.lang — both are "your thread
management could be modernised" search recipes; S2693's remedy is
to move the start() out of the construction path and use an
ExecutorService.

Detection walks getCursor().getPath() from a Thread.start() call
outward to find the enclosing frame:

- J.Lambda                        -> not construction (deferred)
- J.Block with isStatic()         -> static initializer, not construction
- J.MethodDeclaration
  - constructor -> check enclosing class
  - regular method -> not construction
- J.NewClass with body            -> anonymous class, effectively final
- J.ClassDeclaration (first hit)  -> instance field init / init block -> check class

MethodMatcher uses matchOverrides=true so Thread subclasses fire on
myThread.start() too. Records and enums are treated as effectively
final.

Tagged RSPEC-S2693. No CWE — sonar-java doesn't map this rule to
one either.
@steve-aom-elliott
steve-aom-elliott force-pushed the add-rspec-s2693-thread-start-in-constructor branch from a0b9f89 to d6f8415 Compare September 15, 2026 15:53
@steve-aom-elliott
steve-aom-elliott merged commit 4387a78 into main Sep 15, 2026
1 check passed
@steve-aom-elliott
steve-aom-elliott deleted the add-rspec-s2693-thread-start-in-constructor branch September 15, 2026 16:05
@github-project-automation github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

recipe Recipe requested test provided

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant