CAMEL-24301: Fix RouteService NPE when startup-failure exception carries no message - #25205
Open
mayurbm wants to merge 4 commits into
Open
CAMEL-24301: Fix RouteService NPE when startup-failure exception carries no message#25205mayurbm wants to merge 4 commits into
mayurbm wants to merge 4 commits into
Conversation
…as no message RouteService.warmUp() and setUp() passed e.getLocalizedMessage() directly to FailedToStartRouteException, whose constructor calls Objects.requireNonNull on the message argument. When the root cause is a message-less exception (e.g. a bare NullPointerException, StackOverflowError, or a wrapped WSDLException with null description), getLocalizedMessage() returns null and a secondary NPE is thrown from inside the FailedToStartRouteException constructor instead of the intended FailedToStartRouteException. The fix introduces a private extractUsefulMessage helper that walks the cause chain to find the first non-null, non-blank message, falling back to the exception's simple class name. This guarantees the constructor always receives a non-null message and the caller always sees a FailedToStartRouteException with a meaningful description. Adds RouteServiceWarmUpNullMessageTest covering: - message-less NullPointerException on warm-up (regression guard) - message never contains "because: null" - cause-chain walking surfaces the real nested message
apupier
reviewed
Jul 29, 2026
Author
|
JIRA Issue created for the same - https://issues.apache.org/jira/browse/CAMEL-24301 |
mayurbm
changed the title
mayurbm
force-pushed
the
fix/route-service-null-message-on-startup-failure
branch
3 times, most recently
from
July 29, 2026 09:30
c8ec230 to
3a535a9
Compare
apupier
self-requested a review
July 29, 2026 12:12
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
mayurbm
force-pushed
the
fix/route-service-null-message-on-startup-failure
branch
2 times, most recently
from
July 29, 2026 13:17
27b6fd7 to
0f41146
Compare
Author
|
/component-test camel-base-engine camel-core |
Contributor
|
some files requires reformat: you can run mvn clean install on these modules to format them |
mayurbm
force-pushed
the
fix/route-service-null-message-on-startup-failure
branch
2 times, most recently
from
July 29, 2026 16:22
9295732 to
743f5a6
Compare
Author
|
mayurbm
force-pushed
the
fix/route-service-null-message-on-startup-failure
branch
from
July 29, 2026 16:35
743f5a6 to
5de3409
Compare
Contributor
|
compilation failures are reported by the build: |
mayurbm
force-pushed
the
fix/route-service-null-message-on-startup-failure
branch
3 times, most recently
from
July 30, 2026 14:18
0602c89 to
c28a74b
Compare
Contributor
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 548 tested, 29 compile-only — current: 548 all testedMaveniverse Scalpel detected 577 affected modules (current approach: 548).
|
apupier
requested changes
Jul 31, 2026
apupier
left a comment
apupier
left a comment
Contributor
There was a problem hiding this comment.
there is a test to adapt:
[camel-core] [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 3.211 s <<< FAILURE! -- in org.apache.camel.impl.engine.DefaultSupervisingRouteControllerTest
[camel-core] [ERROR] org.apache.camel.impl.engine.DefaultSupervisingRouteControllerTest.testSupervising -- Time elapsed: 3.210 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <Cannot start> but was: <Failed to start route: cake because: Cannot start>
at org.junit.jupiter.api.Assertions.assertEquals(Assertions.java:1199)
at org.apache.camel.impl.engine.DefaultSupervisingRouteControllerTest.testSupervising(DefaultSupervisingRouteControllerTest.java:109)
Author
|
/component-test camel-base-engine camel-core |
mayurbm
force-pushed
the
fix/route-service-null-message-on-startup-failure
branch
from
July 31, 2026 07:57
c28a74b to
cea28c1
Compare
gnodet
requested changes
Aug 2, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
Review Summary
The core fix in RouteService.java (replacing e.getLocalizedMessage() with extractUsefulMessage(e)) is correct and well-motivated. However, the InternalRouteStartupManager changes expand scope beyond the NPE fix, introduce a behavioral change to a public SPI, and break tests in two other modules.
🔴 High: Broken tests in camel-main and camel-spring-xml
The InternalRouteStartupManager changes wrap raw exceptions in FailedToStartRouteException, but only the equivalent test in core/camel-core was updated. Two other test files still assert on the raw exception type/message and will fail:
-
MainSupervisingRouteControllerTest(core/camel-main) — lines 76-77 (testMain) and lines 153-154 (testMainApplicationProperties) assertassertEquals("Cannot start", e.getMessage())andassertInstanceOf(IllegalArgumentException.class, e). After this PR,ewill beFailedToStartRouteExceptionwith a different message. -
SpringSupervisingRouteControllerTest(camel-spring-xml) — lines 72-73 assertassertEquals("Cannot start", e.getMessage())andassertTrue(e instanceof IllegalArgumentException). Same breakage.
🔴 High: InternalRouteStartupManager changes exceed JIRA scope
CAMEL-24301 describes a null-safety bug in RouteService.warmUp() and setUp() where e.getLocalizedMessage() returns null and the FailedToStartRouteException constructor NPEs. The InternalRouteStartupManager code previously did throw e (re-throwing the raw exception) — it never constructed a FailedToStartRouteException, so it had no null-message NPE risk.
The PR introduces FailedToStartRouteException wrapping here for the first time, changing what SupervisingRouteController.getRestartException() returns. This is a public SPI method in org.apache.camel.spi — downstream consumers (health checks, consoles, exception handlers) that rely on the raw exception type will observe a different type.
Recommendation: Consider splitting the PR. The RouteService.java fix is minimal, correct, and ready to merge. The InternalRouteStartupManager wrapping is a separate behavioral change that deserves its own JIRA ticket with complete test updates.
🟡 Medium: extractUsefulMessage duplication
The extractUsefulMessage(Throwable) helper is copy-pasted identically in both RouteService and InternalRouteStartupManager. Consider extracting to a shared location (e.g., a static method in FailedToStartRouteException, or in an existing utility class).
🟡 Medium: testWarmUpWalksCauseChainForMessage doesn't test chain walking
The test throws new RuntimeException(new IllegalStateException(causeMessage)), but RuntimeException(Throwable) sets detailMessage to cause.toString() (non-null). So extractUsefulMessage() returns the outer message on the first iteration without ever walking the chain. To properly test chain walking, the outer exception needs a null or blank message (e.g., a bare NullPointerException with initCause).
🔵 Low: Test conventions
- New test class and methods use
publicmodifier — should be package-private (JUnit 5) - JUnit assertions used instead of project-preferred AssertJ
Positive notes
- The
extractUsefulMessagehelper design (walking the cause chain, falling back to class simple name) is well-thought-out - The previous reviewer's EOL encoding concern appears resolved
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @gnodet
- RouteService.java: fix Javadoc <p> to <p/> per Camel convention, reflow comment lines via Eclipse formatter - RouteServiceWarmUpNullMessageTest.java: replace JUnit assertions with AssertJ, drop public from class/methods (JUnit 5 convention), replace FQCNs with proper imports, reformat via formatter:format + impsort:sort
mayurbm
force-pushed
the
fix/route-service-null-message-on-startup-failure
branch
from
August 3, 2026 06:58
cea28c1 to
eb06378
Compare
The consumer doStart() is called by InternalRouteStartupManager, not by RouteService.warmUp()/setUp(). Move the failure into the endpoint's own doStart() so it is triggered via ServiceHelper.initService(endpoint) inside RouteService.doSetup(), which is the code path the fix actually covers. Also rename test methods to reflect setUp() rather than warmUp().
gnodet
approved these changes
Aug 3, 2026
gnodet
left a comment
gnodet
left a comment
Contributor
There was a problem hiding this comment.
This review was generated by Claude Code, an AI assistant, on behalf of @gnodet.
Thank you for iterating on the feedback -- this revision is much improved.
What was addressed
- Scope reduced to match JIRA -- The
InternalRouteStartupManagerchanges have been removed. The PR now only modifiesRouteService.java, which is exactly where the null-message NPE occurs. This resolves the broken tests incamel-main,camel-spring-xml, andDefaultSupervisingRouteControllerTest. - No more code duplication --
extractUsefulMessageexists only inRouteService. - Test conventions -- Test class and methods are package-private (no
public), and assertions use AssertJ (assertThat,assertThatThrownBy). - EOL encoding -- The diff now only shows changed lines.
Fix assessment
The fix itself is minimal and correct:
extractUsefulMessage(Throwable)walks the cause chain for the first non-null, non-blank message and falls back toe.getClass().getSimpleName()- Both
warmUp()andsetUp()call this helper instead ofe.getLocalizedMessage()directly - The Javadoc on the helper method clearly explains the rationale
- The other call sites of
FailedToStartRouteExceptioninInternalRouteStartupManagerandDefaultRoutesLoaderall use hardcoded string messages (note.getLocalizedMessage()), so they are not affected
Remaining minor suggestion
testSetUpWalksCauseChainForMessage does not truly exercise chain walking (carried forward from the previous review). The endpoint throws new RuntimeException(new IllegalStateException(causeMessage)), but RuntimeException(Throwable) sets detailMessage to cause.toString() which is non-null. So extractUsefulMessage returns the outer message on the first iteration without ever walking the chain.
To properly exercise the chain-walking logic, the outer exception needs a truly null message:
@Override
protected void doStart() {
NullPointerException outer = new NullPointerException();
outer.initCause(new IllegalStateException(causeMessage));
throw outer;
}This is a nice-to-have improvement rather than a blocker -- the chain-walking logic is simple and correct by inspection, and the primary fix path (bare NullPointerException with no message) is well-tested by the first two tests.
Note
CI has not reported any checks on this branch yet. Please ensure CI passes before merging.
…ption(cause) RuntimeException(Throwable) sets detailMessage to cause.toString() which is non-null, so extractUsefulMessage() returned on the first iteration without ever walking the chain. Using a bare NullPointerException with initCause() ensures the outer exception has a null message, forcing the helper to walk to the cause to find the real message.
davsclaus
approved these changes
Aug 3, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
The fix is correct, minimal, and well-scoped to CAMEL-24301.
Fix: extractUsefulMessage(Throwable) walks the cause chain for the first non-null, non-blank message and falls back to e.getClass().getSimpleName() — ensuring FailedToStartRouteException never receives a null message argument (which would NPE in its Objects.requireNonNull constructor call).
Tests: Three tests cover bare NPE, meaningful message content, and cause-chain walking. All follow project conventions (package-private, AssertJ, no Thread.sleep).
Note on the chain-walking test: The carried-forward comment from the previous review round about testSetUpWalksCauseChainForMessage not exercising chain walking appears to be stale — the current revision correctly uses NullPointerException() + initCause() (outer message is genuinely null), so extractUsefulMessage does walk the chain.
Minor (non-blocking):
- Tests 2 and 3 use manual try-catch with cause-chain walking while test 1 uses
assertThatThrownBy— could be simplified for consistency. - Test 1 doesn't call
context.stop()in a finally block while tests 2 and 3 do.
CI is pending — wait for green before merging.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of @davsclaus
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
https://issues.apache.org/jira/browse/CAMEL-24301
Problem
RouteService.warmUp()andsetUp()passe.getLocalizedMessage()directly toFailedToStartRouteException, whose constructor callsObjects.requireNonNullonthat argument. When the root cause is a message-less exception (bare
NullPointerException,StackOverflowError, or a wrapped exception whose inner cause has no text),getLocalizedMessage()returnsnulland a secondaryNullPointerExceptionis thrownfrom inside the constructor instead of the intended
FailedToStartRouteException.This was first observed in Camel 3.14.x where the symptom was
Failed to start route X because of null; the 4.x code fixed that regression butintroduced this adjacent null-safety gap via
Objects.requireNonNullon the message arg.Fix
Adds a private
extractUsefulMessage(Throwable)helper inRouteServicethat walks thecause chain to find the first non-null, non-blank message, falling back to the exception
simple class name. Both
warmUp()andsetUp()now call this helper instead ofe.getLocalizedMessage()directly.Test
RouteServiceWarmUpNullMessageTestadded tocore/camel-corecovers:NullPointerException(no message) on warm-up does not cause a secondary NPE.because: null.is surfaced in the
FailedToStartRouteException.Files changed
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/RouteService.javacore/camel-core/src/test/java/org/apache/camel/impl/engine/RouteServiceWarmUpNullMessageTest.java(new)