gnodet-bot commented on code in PR #28736:
URL: https://github.com/apache/camel/pull/28736#discussion_r4427375787
##########
test-infra/camel-test-infra-artemis/src/main/java/org/apache/camel/test/infra/artemis/services/AbstractArtemisEmbeddedService.java:
##########
@@ -158,7 +158,16 @@ public String password() {
@Override
public void restart() {
Review Comment:
⚠️ **Missing `synchronized`**: `initialize()` and `configure()` are both
`synchronized`; `restart()` should be too. The stop+start sequence is
non-atomic — if two threads call `restart()` concurrently, or if `restart()`
races with `initialize()`, you could end up with multiple interleaved
start/stop calls on the broker.
```suggestion
public synchronized void restart() {
```
##########
test-infra/camel-test-infra-artemis/src/main/java/org/apache/camel/test/infra/artemis/services/AbstractArtemisEmbeddedService.java:
##########
@@ -158,7 +158,16 @@ public String password() {
@Override
public void restart() {
-
+ try {
+ getEmbeddedBrokerService().getActiveMQServer().stop(false);
+ getEmbeddedBrokerService().getActiveMQServer().start();
+ if
(!embeddedBrokerService.getActiveMQServer().waitForActivation(20,
TimeUnit.SECONDS)) {
Review Comment:
💡 **Nit**: lines 162–163 access the server via
`getEmbeddedBrokerService().getActiveMQServer()`, but this line uses the field
`embeddedBrokerService` directly. Functionally equivalent (the getter just
returns the field), but inconsistent within the same method. Pick one style.
##########
test-infra/camel-test-infra-artemis/src/main/java/org/apache/camel/test/infra/artemis/services/AbstractArtemisEmbeddedService.java:
##########
@@ -158,7 +158,16 @@ public String password() {
@Override
public void restart() {
-
+ try {
+ getEmbeddedBrokerService().getActiveMQServer().stop(false);
+ getEmbeddedBrokerService().getActiveMQServer().start();
Review Comment:
⚠️ **Wrong abstraction level**: `shutdown()` calls
`embeddedBrokerService.stop()` (via the `EmbeddedActiveMQ` wrapper). This
`restart()` calls `getActiveMQServer().stop(false)`/`.start()` directly,
bypassing the wrapper. The wrapper may hold internal state (acceptors,
interceptors) that `EmbeddedActiveMQ.stop()` tears down properly. Using the
same abstraction level as `initialize()` and `shutdown()` is safer:
```suggestion
embeddedBrokerService.stop();
embeddedBrokerService.start();
```
Note: if you keep the server-level calls, the `waitForActivation` on line
164 must stay on `getActiveMQServer()` (not on `embeddedBrokerService`) since
`EmbeddedActiveMQ` has no such method.
--
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]
