allthingssecurity commented on code in PR #26939:
URL: https://github.com/apache/camel/pull/26939#discussion_r4129056468
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/debugger/DefaultBacklogDebugger.java:
##########
@@ -239,7 +239,17 @@ public boolean hasBreakpoint(String nodeId) {
@Override
public void setSuspendMode(boolean suspendMode) {
+ boolean changed = this.suspendMode != suspendMode;
this.suspendMode = suspendMode;
+ if (changed) {
+ if (suspendMode) {
+ // wait for a debugger to attach (as when suspend mode is set
from the environment)
+ detach();
+ } else {
+ // do not keep messages waiting for a debugger that is no
longer expected
+ resumeMessageProcessing();
Review Comment:
camel-main calls `debugger.setSuspendMode(config.isWaitForAttach())`
unconditionally in `DefaultConfigurationConfigurer.configureBacklogDebugger`
(whenever `camel.debug.enabled` or `standby` is set), and `waitForAttach`
defaults to `false`. When the suspend was requested with
`CAMEL_DEBUGGER_SUSPEND` or `-Dorg.apache.camel.debugger.suspend=true`, the
constructor has already called `detach()`. That default `false` now counts as a
change and resumes processing, so the environment setting is dropped silently.
Before this PR the same combination left messages waiting forever, so this is
an improvement, but neither behaviour honours the environment. Could the
configurer call `setSuspendMode` only when `waitForAttach` is `true`, or OR it
with the current `isSuspendMode()`?
Also, `camel.debug.waitForAttach=true` now really suspends processing until
a debugger attaches, where before it did nothing, so an app that has it set
will wait at startup after upgrading. That seems worth a line in the upgrade
guide.
_Review by Claude Code on behalf of allthingssecurity_
##########
core/camel-base-engine/src/main/java/org/apache/camel/impl/debugger/DefaultBacklogTracerEventMessage.java:
##########
@@ -372,34 +372,36 @@ public String toXml(int indent) {
sb.append(prefix).append("
<done>").append(isDone()).append("</done>\n");
sb.append(prefix).append("
<failed>").append(isFailed()).append("</failed>\n");
if (getLocation() != null) {
- sb.append(prefix).append("
<location>").append(getLocation()).append("</location>\n");
+ sb.append(prefix).append("
<location>").append(StringHelper.xmlEncode(getLocation())).append("</location>\n");
}
// route id is optional and we then use an empty value for no route id
sb.append(prefix).append(" <routeId>").append(routeId != null ?
routeId : "").append("</routeId>\n");
Review Comment:
Nit: `routeId` and `fromRouteId` here, and `routeId` again in the `<toNode>`
fallback at line 389, are still written without `StringHelper.xmlEncode`. A
route id is free text (`routeId("a&b")`), so the dump can still be malformed.
_Review by Claude Code on behalf of allthingssecurity_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
