gnodet-bot commented on code in PR #27472:
URL: https://github.com/apache/camel/pull/27472#discussion_r4214991631
##########
dsl/camel-jbang/camel-jbang-core/src/main/java/org/apache/camel/dsl/jbang/core/commands/infra/InfraRun.java:
##########
@@ -179,6 +200,25 @@ protected Integer doRun(String testService, String
testServiceImplementation, Te
if (noUi) {
System.setProperty("camel.infra.ui", "false");
}
+ // the service resolves its own properties from the system properties
first, so they must be set
+ // before it is instantiated
+ Map<String, String> replacedProperties =
applyServiceProperties(parsedProperties);
+ try {
+ return startService(testService, testServiceImplementation,
testInfraService, cl, serviceInterface,
+ serviceImpl, replacedProperties);
+ } finally {
+ // the service may never have started, so this and not only the
shutdown is where a JVM that keeps
+ // running, such as the TUI or a test, gets its properties back
+ restoreProperties(replacedProperties);
+ clearInfraProperties();
Review Comment:
On the normal shutdown path, `restoreProperties` + `clearInfraProperties`
run **twice**: once inside `shutdownInfra` (called from `startService`), and
once again here in `doRun`'s `finally` block.
The comment explains the intent — this `finally` handles the case where the
service never started (no JVM shutdown hook). That is a real edge case, but the
two sites are now doing the same work on the normal path too.
This is functionally harmless (`clearProperty` and `setProperty` are
idempotent), but it makes the cleanup lifecycle harder to reason about. A
future maintainer reading `shutdownInfra` will wonder whether the `doRun`
finally is supposed to run or not. Consider either:
- Having the `finally` block be guarded (e.g., only run if `startService`
never called `shutdownInfra`), or
- Keeping `shutdownInfra` as the sole cleanup site, and having `doRun` only
call it directly for the early-exit case
Not a blocker — the code is correct as written.
--
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]
