GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — build: add SpotBugs plugin and fix all reported findings by csviri · Pull Request #3390 · operator-framework/java-operator-sdk

Conversation

csviri commented May 29, 2026

Copy link

Copy Markdown

Collaborator

Add the SpotBugs Maven plugin (effort=Max, threshold=Medium) with a
check execution bound to the verify phase across all modules, plus a
documented spotbugs-exclude.xml for intentional/false-positive findings.

Real bug fixes:

ExecutorServiceManager: synchronize start()/stop() so started and
configurationService are accessed consistently with the already-locked
lazyInitWorkflowExecutorService() (IS2_INCONSISTENT_SYNC,
AT_STALE_THREAD_WRITE_OF_PRIMITIVE).
AbstractEventSourceHolderDependentResource: make isCacheFillerEventSource
volatile (written under lock, read from reconcile threads).
ManagedInformerEventSource: make cache and controllerConfiguration
volatile (assigned under lock in start(), read lock-free).
AbstractOperatorExtension: the builder's namespaceDeleteTimeout was
configurable but never plumbed into the extension, so the namespace
deletion timeout always used the default. Threaded it through the
constructors of both LocallyRunOperatorExtension and
ClusterDeployedOperatorExtension.
MicrometerMetrics.addMetadataTags(): guard against null metadata, which
incrementCounter() already tolerates (latent NPE, NP_NULL_PARAM_DEREF).
Bootstrapper.addTemplatedFile(): close the Writer via try-with-resources
and write as UTF-8 (OBL_UNSATISFIED_OBLIGATION, DM_DEFAULT_ENCODING).

Default-encoding fixes (DM_DEFAULT_ENCODING), all pinned to UTF-8:

LocallyRunOperatorExtension CRD apply/delete byte conversions.
AccumulativeMappingWriter reader and PrintWriter.
ClassMappingProvider InputStreamReader.
sample mysql-schema Secret/Schema Base64 encode/decode.
sample tomcat-operator WebappReconciler ByteArrayOutputStream.toString().

Sample cleanups:

TomcatReconciler: avoid unbox-then-rebox in a log argument
(BX_UNBOXING_IMMEDIATELY_REBOXED).
OperationsSampleOperator: make the config path overridable via the
CONFIG_PATH env var instead of a hardcoded absolute path
(DMI_HARDCODED_ABSOLUTE_FILENAME).

Suppressed in spotbugs-exclude.xml (intentional / false positives):
EI_EXPOSE_REP(2) and CT_CONSTRUCTOR_THROW project-wide, plus DM_EXIT in
LeaderElectionManager, NP_BOOLEAN_RETURN_NULL in BooleanWithUndefined,
EQ_DOESNT_OVERRIDE_EQUALS in GroupVersionKindPlural, and
SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR in
SSABasedGenericKubernetesResourceMatcher and ConfigLoader.

Co-Authored-By: Claude Opus 4.8 (1M context) [email protected]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Copilot AI review requested due to automatic review settings

May 29, 2026 14:14

openshift-ci Bot requested review from metacosm and xstefank

May 29, 2026 14:15

Copilot started reviewing on behalf of csviri

May 29, 2026 14:15

View session

csviri force-pushed the find-bugs branch from 3ef06fe to 17836d5 Compare

May 29, 2026 14:18

Copilot AI reviewed

May 29, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces SpotBugs to the Maven build (run on verify) and addresses the resulting findings across core framework code, the JUnit extensions, the bootstrapper plugin, and samples—primarily around concurrency visibility/synchronization and deterministic UTF-8 encoding.

Changes:

Add and configure the SpotBugs Maven plugin (plus a repository-wide spotbugs-exclude.xml for intentional/false positives).
Fix concurrency/visibility issues (e.g., synchronized lifecycle methods and volatile fields) and a latent NPE in Micrometer tag handling.
Pin default-encoding-sensitive I/O to UTF-8 and close resources correctly (try-with-resources).

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`pom.xml`	Adds and configures the SpotBugs Maven plugin + version management.
`spotbugs-exclude.xml`	Introduces a centralized SpotBugs exclude filter for known intentional/false positives.
`operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java`	Synchronization changes for lifecycle methods to address SpotBugs concurrency findings.
`operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java`	Marks a cross-thread-read flag as `volatile` for visibility.
`operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java`	Marks fields as `volatile` to ensure safe publication after `start()`.
`operator-framework-junit/src/main/java/io/javaoperatorsdk/operator/junit/AbstractOperatorExtension.java`	Plumbs namespace deletion timeout through the base extension.
`operator-framework-junit/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java`	Plumbs timeout and pins CRD YAML byte conversions to UTF-8.
`operator-framework-junit/src/main/java/io/javaoperatorsdk/operator/junit/ClusterDeployedOperatorExtension.java`	Plumbs timeout through constructors/builders.
`operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ClassMappingProvider.java`	Pins `InputStreamReader` decoding to UTF-8.
`operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AccumulativeMappingWriter.java`	Pins reader/writer encoding to UTF-8 for generated mapping files.
`micrometer-support/src/main/java/io/javaoperatorsdk/operator/monitoring/micrometer/MicrometerMetrics.java`	Guards against null `metadata` when extracting GVK tags.
`bootstrapper-maven-plugin/src/main/java/io/javaoperatorsdk/boostrapper/Bootstrapper.java`	Writes templated files as UTF-8 and closes the writer via try-with-resources.
`sample-operators/operations/src/main/java/io/javaoperatorsdk/operator/sample/operations/OperationsSampleOperator.java`	Makes the sample config path overridable via `CONFIG_PATH`.
`sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/dependent/SecretDependentResource.java`	Pins Base64 input bytes to UTF-8.
`sample-operators/mysql-schema/src/main/java/io/javaoperatorsdk/operator/sample/dependent/SchemaDependentResource.java`	Pins Base64 decode path to UTF-8.
`sample-operators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/TomcatReconciler.java`	Avoids unbox/rebox in log argument selection.
`sample-operators/tomcat-operator/src/main/java/io/javaoperatorsdk/operator/sample/WebappReconciler.java`	Pins `ByteArrayOutputStream` string conversion to UTF-8.

Comments suppressed due to low confidence (2)

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ExecutorServiceManager.java:161

stop() is synchronized while it waits for executor termination, which can block in-flight reconciliation threads that try to call back into ExecutorServiceManager (e.g., via DefaultContext#getWorkflowExecutorService() -> workflowExecutorService() -> synchronized lazyInitWorkflowExecutorService()), preventing tasks from completing and delaying/shrugging shutdown. Avoid holding the monitor while awaiting termination by snapshotting state under synchronization, then performing shutdown outside the synchronized block.

  public synchronized void stop(Duration gracefulShutdownTimeout) {
    try {
      log.debug("Closing executor");
      var parallelExec = Executors.newFixedThreadPool(3);
      parallelExec.invokeAll(
          List.of(
              shutdown(executor, gracefulShutdownTimeout),
              shutdown(workflowExecutor, gracefulShutdownTimeout),
              shutdown(cachingExecutorService, gracefulShutdownTimeout)));
      workflowExecutor = null;
      parallelExec.shutdownNow();
      started = false;
    } catch (InterruptedException e) {
      log.debug("Exception closing executor: {}", e.getLocalizedMessage());
      Thread.currentThread().interrupt();
    }
  }

operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AccumulativeMappingWriter.java:70

flush() manually closes the PrintWriter; if an exception occurs while iterating/writing, the writer may not be closed. Using try-with-resources makes the close unconditional and simplifies the method.

              new InputStreamReader(readonlyResource.openInputStream(), StandardCharsets.UTF_8))) {
        final var existingLines =
            bufferedReader
                .lines()
                .map(l -> l.split(","))
                .collect(Collectors.toMap(parts -> parts[0], parts -> parts[1]));
        mappings.putAll(existingLines);
      }
    } catch (IOException e) {
    }
    return this;
  }

  /** Add a new mapping */
  public AccumulativeMappingWriter add(String key, String value) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

...hema/src/main/java/io/javaoperatorsdk/operator/sample/dependent/SchemaDependentResource.java Resolved

Uh oh!

There was an error while loading. Please reload this page.

csviri requested a review from Copilot

May 29, 2026 14:35

Copilot started reviewing on behalf of csviri

May 29, 2026 14:35

View session

Copilot AI reviewed

May 29, 2026

View reviewed changes

Copilot AI left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

metacosm approved these changes

May 29, 2026

View reviewed changes

xstefank approved these changes

Jun 1, 2026

View reviewed changes

Comment thread

...ns/src/main/java/io/javaoperatorsdk/operator/sample/operations/OperationsSampleOperator.java Resolved

Uh oh!

There was an error while loading. Please reload this page.

csviri and others added 2 commits

June 15, 2026 17:34


          build: add SpotBugs plugin and fix all reported findings

681ca6b

Add the SpotBugs Maven plugin (effort=Max, threshold=Medium) with a
`check` execution bound to the `verify` phase across all modules, plus a
documented `spotbugs-exclude.xml` for intentional/false-positive findings.

Real bug fixes:

- ExecutorServiceManager: synchronize start()/stop() so `started` and
  `configurationService` are accessed consistently with the already-locked
  lazyInitWorkflowExecutorService() (IS2_INCONSISTENT_SYNC,
  AT_STALE_THREAD_WRITE_OF_PRIMITIVE).
- AbstractEventSourceHolderDependentResource: make `isCacheFillerEventSource`
  volatile (written under lock, read from reconcile threads).
- ManagedInformerEventSource: make `cache` and `controllerConfiguration`
  volatile (assigned under lock in start(), read lock-free).
- AbstractOperatorExtension: the builder's `namespaceDeleteTimeout` was
  configurable but never plumbed into the extension, so the namespace
  deletion timeout always used the default. Threaded it through the
  constructors of both LocallyRunOperatorExtension and
  ClusterDeployedOperatorExtension.
- MicrometerMetrics.addMetadataTags(): guard against null `metadata`, which
  incrementCounter() already tolerates (latent NPE, NP_NULL_PARAM_DEREF).
- Bootstrapper.addTemplatedFile(): close the Writer via try-with-resources
  and write as UTF-8 (OBL_UNSATISFIED_OBLIGATION, DM_DEFAULT_ENCODING).

Default-encoding fixes (DM_DEFAULT_ENCODING), all pinned to UTF-8:

- LocallyRunOperatorExtension CRD apply/delete byte conversions.
- AccumulativeMappingWriter reader and PrintWriter.
- ClassMappingProvider InputStreamReader.
- sample mysql-schema Secret/Schema Base64 encode/decode.
- sample tomcat-operator WebappReconciler ByteArrayOutputStream.toString().

Sample cleanups:

- TomcatReconciler: avoid unbox-then-rebox in a log argument
  (BX_UNBOXING_IMMEDIATELY_REBOXED).
- OperationsSampleOperator: make the config path overridable via the
  CONFIG_PATH env var instead of a hardcoded absolute path
  (DMI_HARDCODED_ABSOLUTE_FILENAME).

Suppressed in spotbugs-exclude.xml (intentional / false positives):
EI_EXPOSE_REP(2) and CT_CONSTRUCTOR_THROW project-wide, plus DM_EXIT in
LeaderElectionManager, NP_BOOLEAN_RETURN_NULL in BooleanWithUndefined,
EQ_DOESNT_OVERRIDE_EQUALS in GroupVersionKindPlural, and
SING_SINGLETON_HAS_NONPRIVATE_CONSTRUCTOR in
SSABasedGenericKubernetesResourceMatcher and ConfigLoader.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>


          fix from code review

ab3ee97

Signed-off-by: Attila Mészáros <[email protected]>

csviri force-pushed the find-bugs branch from b06f750 to ab3ee97 Compare

June 15, 2026 15:35

csviri added 2 commits

June 15, 2026 17:39


          fix IT

e84d7fc

Signed-off-by: Attila Mészáros <[email protected]>


          fix from cr

d732378

Signed-off-by: Attila Mészáros <[email protected]>

csviri merged commit 2a3dff4 into operator-framework:main

Jun 16, 2026

27 checks passed

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

Copilot code review Copilot

Copilot review effort

Applies to this pull request for everyone.Learn more about Copilot code review.

Copilot left review comments

metacosm

metacosm approved these changes

xstefank

xstefank approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.