davsclaus commented on code in PR #26533: URL: https://github.com/apache/camel/pull/26533#discussion_r4034478079
########## components/camel-vertx/camel-vertx-websocket/src/test/java/org/apache/camel/component/vertx/websocket/VertxWebsocketMultiConsumerLifecycleTest.java: ########## @@ -0,0 +1,92 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.camel.component.vertx.websocket; + +import java.util.Map; + +import org.apache.camel.RoutesBuilder; +import org.apache.camel.builder.RouteBuilder; +import org.apache.camel.component.mock.MockEndpoint; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; Review Comment: Non-blocking, house style: all 16 existing test classes in this module use `org.junit.jupiter.api.Assertions`, and `CLAUDE.md` asks not to mix styles within a module ("if they are predominantly JUnit assertions, write new tests in that same JUnit style rather than introducing AssertJ as an outlier"). The three assertions map 1:1 — `assertEquals(1, hostRegistry().size())` / `assertTrue(hostRegistry().isEmpty())` — which would also let the `assertj-core` addition in `pom.xml` go away. Sibling `camel-vertx-http` does use AssertJ, so this isn't unprecedented in the parent folder; fine to keep if you prefer, just flagging the module convention. ```suggestion import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; ``` ########## components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketHost.java: ########## @@ -51,7 +51,8 @@ public class VertxWebsocketHost { private final VertxWebsocketHostConfiguration hostConfiguration; private final VertxWebsocketHostKey hostKey; - private final Map<String, Route> routeRegistry = new HashMap<>(); + // routes are added and removed as consumers start and stop, which the route controller can do concurrently + private final Map<String, Route> routeRegistry = new ConcurrentHashMap<>(); Review Comment: Non-blocking: the `ConcurrentHashMap` makes the individual `put`/`remove` calls safe, but `disconnect()` is still `remove(path) → isEmpty() → stop()`, and `stop()` does its own `routeRegistry.clear()`. A concurrent `connect()`+`start()` on the same host can add a route between `isEmpty()` and `clear()`, which would silently drop that route while the host stays in the component registry with `server == null`. That race is pre-existing (and rarer than the bug this PR fixes — the default route controller starts/stops routes sequentially), so I would not hold this PR for it. But if concurrent start/stop is the motivation stated in this comment, a `synchronized` on `connect`/`disconnect` (or a follow-up JIRA) is the actual fix; on its own the map change is more a tidy-up than a guarantee, so the comment may slightly overstate it. -- 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]
