You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "oscerd (via GitHub)" <gi...@apache.org> on 2026/09/17 07:45:56 UTC
[PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
oscerd opened a new pull request, #26533:
URL: https://github.com/apache/camel/pull/26533
Found by a source audit of `components/camel-vertx`.
### The problem
`VertxWebsocketHostKey` is `{host, port}` only, so every consumer on the same port shares one
`VertxWebsocketHost` (`VertxWebsocketComponent:167`, `computeIfAbsent`). But the host was removed from the
registry by whichever consumer stopped first:
```java
VertxWebsocketHost vertxWebsocketHost = vertxHostRegistry.remove(hostKey);
if (vertxWebsocketHost != null) {
vertxWebsocketHost.disconnect(configuration.getWebsocketURI().getPath());
}
```
With consumers on `/test/a` and `/test/b` sharing a port:
* stopping `a` removes the shared host from the registry and unregisters route `a`. The host correctly keeps
its server up, because `b` is still in its own `routeRegistry`;
* stopping `b` then finds nothing — `remove` returns null, the guard skips, and **route `b` is never removed
and its server never stops**, outliving every Camel route;
* starting a consumer on that host and port again builds a *second* host with a new Router and calls
`start()` on a port the first server still holds.
`doStop` closes Vert.x only when Camel created it, so with an application-supplied `Vertx` or `Router` —
Spring Boot, Quarkus — nothing tears the server down at all.
Multiple consumers per port is supported and tested behaviour (`VertxWebsocketMultiConsumerTest`), so this
is reachable in an ordinary configuration.
### The change
The host is looked up rather than removed, and leaves the registry only once it serves no consumer — which
is the same moment it stops its own server. `isServingConsumers()` is new and additive; `disconnect` keeps
its signature, since `VertxWebsocketHost` is public.
Two smaller problems in the same lifecycle go with it:
* `routeRegistry` becomes a `ConcurrentHashMap`. It is mutated by `connect`/`disconnect`, i.e. as the route
controller starts and stops consumers, which it can do concurrently; the field immediately below it was
already a `CopyOnWriteArrayList` carrying a `// thread-safe` comment, so the difference read as an
oversight rather than a decision.
* `disconnect` no longer calls `route.remove()` on a null route when handed a path it does not serve.
### Tests
`VertxWebsocketMultiConsumerLifecycleTest` (new, 3 cases) covers two consumers sharing a port: the host
survives the first stop and the surviving consumer still receives; the host is gone after the last stop; and
stop/start/stop of one consumer leaves the registry intact.
**Checked that the tests actually catch this**, rather than trusting green: with the component change
reverted, 2 of the 3 fail (`theHostOutlivesTheFirstConsumerToStop`, `stoppingAConsumerTwiceIsHarmless`). The
third passes either way — it is the degenerate case, kept because it pins the intended end state.
`mvn clean install -DskipITs` on `components/camel-vertx` is green — 88 tests. Full reactor
`mvn clean install -DskipTests -DskipITs -Dquickly` green.
`org.assertj:assertj-core` was not on this module's test classpath and is added.
---
_Claude Code on behalf of oscerd_
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #26533:
URL: https://github.com/apache/camel/pull/26533#discussion_r4034444932
##########
components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketHost.java:
##########
@@ -162,7 +163,9 @@ public void connect(VertxWebsocketConsumer consumer) {
public void disconnect(String path) {
LOG.info("Disconnected consumer for path {}", path);
Route route = routeRegistry.remove(path);
- route.remove();
+ if (route != null) {
+ route.remove();
+ }
if (routeRegistry.isEmpty()) {
Review Comment:
Test inline comment
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26533:
URL: https://github.com/apache/camel/pull/26533#issuecomment-5711267793
<!-- ci-tested-modules -->
:test_tube: **CI tested the following changed modules:**
- `components/camel-vertx/camel-vertx-websocket`
---
<details><summary>:microscope: Scalpel shadow comparison — Scalpel: 10 tested, 27 compile-only — current: 10 all tested</summary>
[Maveniverse Scalpel](https://github.com/maveniverse/scalpel) detected **37 affected modules** (current approach: 10).
<details><summary>:warning: Modules only in Scalpel (27)</summary>
- `apache-camel`
- `camel-allcomponents`
- `camel-catalog`
- `camel-catalog-console`
- `camel-catalog-maven`
- `camel-catalog-suggest`
- `camel-componentdsl`
- `camel-endpointdsl`
- `camel-endpointdsl-support`
- `camel-itest`
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-kamelet-main`
- `camel-launcher`
- `camel-report-maven-plugin`
- `camel-route-parser`
- `camel-yaml-dsl`
- `camel-yaml-dsl-deserializers`
- `camel-yaml-dsl-maven-plugin`
- `coverage`
- `docs`
- `dummy-component`
</details>
**Skip-tests mode would test 10 modules** (1 direct + 9 downstream), **skip tests for 27** (generated code, meta-modules)
<details><summary>Modules Scalpel would test (10)</summary>
- `camel-ibm-watsonx-ai`
- `camel-jbang-mcp`
- `camel-jbang-plugin-mcp`
- `camel-jbang-plugin-route-parser`
- `camel-jbang-plugin-tui`
- `camel-jbang-plugin-validate`
- `camel-launcher-container`
- `camel-vertx-websocket`
- `camel-yaml-dsl-validator`
- `camel-yaml-dsl-validator-maven-plugin`
</details>
<details><summary>Modules with tests skipped (27)</summary>
- `apache-camel`
- `camel-allcomponents`
- `camel-catalog`
- `camel-catalog-console`
- `camel-catalog-maven`
- `camel-catalog-suggest`
- `camel-componentdsl`
- `camel-endpointdsl`
- `camel-endpointdsl-support`
- `camel-itest`
- `camel-jbang-core`
- `camel-jbang-it`
- `camel-jbang-main`
- `camel-jbang-plugin-edit`
- `camel-jbang-plugin-generate`
- `camel-jbang-plugin-kubernetes`
- `camel-jbang-plugin-test`
- `camel-kamelet-main`
- `camel-launcher`
- `camel-report-maven-plugin`
- `camel-route-parser`
- `camel-yaml-dsl`
- `camel-yaml-dsl-deserializers`
- `camel-yaml-dsl-maven-plugin`
- `coverage`
- `docs`
- `dummy-component`
</details>
> :information_source: Shadow mode — Scalpel observes but does not affect test execution. [Learn more](https://github.com/maveniverse/scalpel)
</details>
<details><summary>All tested modules (37 modules, 5m 33s total)</summary>
**Total reactor time:** 5m 33s
| Module | Duration | Status |
| --- | --- | --- |
| Camel :: Launcher | 51.8s | SUCCESS |
| Camel :: JBang :: MCP | 42.5s | SUCCESS |
| Camel :: Vert.x :: WebSocket | 34.3s | SUCCESS |
| Camel :: JBang :: Plugin :: TUI | 33.5s | SUCCESS |
| Camel :: Catalog :: Camel Catalog | 26.5s | SUCCESS |
| Camel :: Component DSL | 21.6s | SUCCESS |
| Camel :: Docs | 16.9s | SUCCESS |
| Camel :: JBang :: Plugin :: Kubernetes | 16.2s | SUCCESS |
| Camel :: Catalog :: Camel Report Maven Plugin | 10.0s | SUCCESS |
| Camel :: Catalog :: Camel Route Parser | 8.6s | SUCCESS |
| Camel :: Kamelet Main | 8.2s | SUCCESS |
| Camel :: JBang :: Plugin :: Testing | 8.1s | SUCCESS |
| Camel :: YAML DSL :: Validator | 7.9s | SUCCESS |
| Camel :: IBM :: watsonx.ai | 5.9s | SUCCESS |
| Camel :: YAML DSL :: Deserializers | 5.9s | SUCCESS |
| Camel :: JBang :: Plugin :: Validate | 5.2s | SUCCESS |
| Camel :: All Components Sync point | 5.1s | SUCCESS |
| Camel :: YAML DSL :: Validator Maven Plugin | 4.6s | SUCCESS |
| Camel :: YAML DSL :: Maven Plugins | 3.2s | SUCCESS |
| Camel :: Catalog :: Maven | 2.8s | SUCCESS |
| Camel :: Catalog :: Suggest (deprecated) | 2.4s | SUCCESS |
| Camel :: JBang :: Plugin :: Edit | 1.5s | SUCCESS |
| Camel :: Coverage | 1.4s | SUCCESS |
| Camel :: Assembly | 1.2s | SUCCESS |
| Camel :: Catalog :: Dummy Component | 1.1s | SUCCESS |
| Camel :: JBang :: Plugin :: Generate | 1.1s | SUCCESS |
| Camel :: JBang :: Integration tests | 1.0s | SUCCESS |
| Camel :: JBang :: Main | 0.9s | SUCCESS |
| Camel :: JBang :: Plugin :: MCP | 0.8s | SUCCESS |
| Camel :: Catalog :: Console | 0.8s | SUCCESS |
| Camel :: Endpoint DSL :: Support | 0.8s | SUCCESS |
| Camel :: JBang :: Plugin :: Route Parser | 0.7s | SUCCESS |
| Camel :: Launcher :: Container | 0.7s | SUCCESS |
| Camel :: Endpoint DSL | n/a | |
| Camel :: Integration Tests | n/a | |
| Camel :: JBang :: Core | n/a | |
| Camel :: YAML DSL | n/a | |
**Top 20 slowest modules:**
- `Camel :: Launcher` (51.8s)
- `Camel :: JBang :: MCP` (42.5s)
- `Camel :: Vert.x :: WebSocket` (34.3s)
- `Camel :: JBang :: Plugin :: TUI` (33.5s)
- `Camel :: Catalog :: Camel Catalog` (26.5s)
- `Camel :: Component DSL` (21.6s)
- `Camel :: Docs` (16.9s)
- `Camel :: JBang :: Plugin :: Kubernetes` (16.2s)
- `Camel :: Catalog :: Camel Report Maven Plugin` (10.0s)
- `Camel :: Catalog :: Camel Route Parser` (8.6s)
- `Camel :: Kamelet Main` (8.2s)
- `Camel :: JBang :: Plugin :: Testing` (8.1s)
- `Camel :: YAML DSL :: Validator` (7.9s)
- `Camel :: IBM :: watsonx.ai` (5.9s)
- `Camel :: YAML DSL :: Deserializers` (5.9s)
- `Camel :: JBang :: Plugin :: Validate` (5.2s)
- `Camel :: All Components Sync point` (5.1s)
- `Camel :: YAML DSL :: Validator Maven Plugin` (4.6s)
- `Camel :: YAML DSL :: Maven Plugins` (3.2s)
- `Camel :: Catalog :: Maven` (2.8s)
</details>
---
:gear: [View full build and test results](https://github.com/apache/camel/actions/runs/35196179310)
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #26533:
URL: https://github.com/apache/camel/pull/26533#discussion_r4034461152
##########
components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketHost.java:
##########
@@ -162,7 +163,9 @@ public void connect(VertxWebsocketConsumer consumer) {
public void disconnect(String path) {
LOG.info("Disconnected consumer for path {}", path);
Route route = routeRegistry.remove(path);
- route.remove();
+ if (route != null) {
+ route.remove();
+ }
if (routeRegistry.isEmpty()) {
Review Comment:
⚠️ **Non-atomic check-then-act: double `stop()` is possible under concurrent disconnects.**
`routeRegistry` is now a `ConcurrentHashMap`, so `.remove()` and `.isEmpty()` are each individually atomic — but the compound sequence `remove(path) → isEmpty() → stop()` is not. The commit message states the route controller stops consumers *possibly concurrently*, so this race is reachable:
1. T1: `routeRegistry.remove("/a")` → `{"/b"}` remains
2. T2: `routeRegistry.remove("/b")` → `{}` empty
3. T2: `isEmpty()` = true → calls `stop()`
4. T1: `isEmpty()` = true → calls `stop()` again (server is already closing)
`stop()` is not synchronized and `server` is not `volatile`, so both threads can see `server != null` and both call `server.close()` on the same `HttpServer` object concurrently.
Fix: synchronize the empty-check/stop pair — lifecycle operations, not hot paths:
##########
components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketHost.java:
##########
@@ -236,6 +239,14 @@ public void stop() throws ExecutionException, InterruptedException {
port = VertxWebsocketConstants.DEFAULT_VERTX_SERVER_PORT;
}
+ /**
+ * Whether this host still serves any consumer. Every consumer bound to the same host and port shares one instance,
+ * so the host outlives the first consumer that stops, and only once the last one goes is its server stopped.
+ */
+ public boolean isServingConsumers() {
Review Comment:
⚠️ **`server` field is not `volatile` — data race between `start()` and `stop()`.**
`start()` checks `if (server == null)` and `stop()` checks `if (server != null)` then sets `this.server = null` in a `finally` block. None of these accesses are on a `volatile` field or inside a synchronized block. In the concurrent-lifecycle scenario the commit describes, there is no happens-before edge between `stop()` writing `null` and a subsequent `start()` reading it.
Minimal fix: declare `private volatile HttpServer server;` at line 58.
##########
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;
+
+/**
+ * Consumers bound to the same host and port share one {@link VertxWebsocketHost}, so stopping one of them must leave
+ * the others able to serve and, later, to disconnect.
+ */
+public class VertxWebsocketMultiConsumerLifecycleTest extends VertxWebSocketTestSupport {
+
+ private Map<VertxWebsocketHostKey, VertxWebsocketHost> hostRegistry() {
+ return context.getComponent("vertx-websocket", VertxWebsocketComponent.class).getVertxHostRegistry();
+ }
+
+ @Test
+ void theHostOutlivesTheFirstConsumerToStop() throws Exception {
+ assertThat(hostRegistry()).hasSize(1);
+
+ context.getRouteController().stopRoute("a");
+
+ // the host still serves route b, so it must still be reachable - it used to be dropped from the
+ // registry here, which left b unable to ever disconnect and its server running for good
+ assertThat(hostRegistry()).hasSize(1);
+
+ MockEndpoint mockEndpoint = getMockEndpoint("mock:result");
+ mockEndpoint.expectedBodiesReceived("Hello b");
+
+ template.sendBody("vertx-websocket:localhost:" + port + "/test/b", "b");
+
+ mockEndpoint.assertIsSatisfied();
+ }
+
+ @Test
+ void theHostGoesWithTheLastConsumerToStop() throws Exception {
+ assertThat(hostRegistry()).hasSize(1);
+
+ context.getRouteController().stopRoute("a");
+ context.getRouteController().stopRoute("b");
+
+ assertThat(hostRegistry()).isEmpty();
+ }
+
+ @Test
+ void stoppingAConsumerTwiceIsHarmless() throws Exception {
Review Comment:
💡 **Test gap: the concurrent-stop scenario that motivates `ConcurrentHashMap` is not exercised.**
`stoppingAConsumerTwiceIsHarmless` is sequential — it verifies idempotence of a single-threaded stop/start/stop cycle, not thread safety. The race that justifies switching from `HashMap` to `ConcurrentHashMap` — two consumers on the same host stopped simultaneously — has no test. Add a test that stops routes `"a"` and `"b"` from two concurrent threads and asserts the host registry is empty without exceptions.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "oscerd (via GitHub)" <gi...@apache.org>.
oscerd commented on code in PR #26533:
URL: https://github.com/apache/camel/pull/26533#discussion_r4044698538
##########
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:
You are right, and the comment was overstating it — the map change on its own guaranteed very little, and `stop()` clearing the registry under a concurrent `connect()` is exactly the hole.
Rather than leave it as a tidy-up with a follow-up JIRA, the lifecycle methods are now synchronized and the registry is a plain `HashMap` again, documented as guarded by that monitor. `disconnectConsumer` drops the host with `computeIfPresent` against the same key `connectConsumer` computes on, so a consumer connecting at that moment either keeps the host or gets a fresh one. The misleading comment is gone.
_Claude Code on behalf of oscerd_
##########
components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketHost.java:
##########
@@ -162,7 +163,9 @@ public void connect(VertxWebsocketConsumer consumer) {
public void disconnect(String path) {
LOG.info("Disconnected consumer for path {}", path);
Route route = routeRegistry.remove(path);
- route.remove();
+ if (route != null) {
+ route.remove();
+ }
if (routeRegistry.isEmpty()) {
Review Comment:
No action — this thread is a placeholder comment with no content. Resolving to clear the merge gate; happy to reopen if something was meant to land here.
_Claude Code on behalf of oscerd_
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "oscerd (via GitHub)" <gi...@apache.org>.
oscerd commented on code in PR #26533:
URL: https://github.com/apache/camel/pull/26533#discussion_r4044696975
##########
components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketHost.java:
##########
@@ -162,7 +163,9 @@ public void connect(VertxWebsocketConsumer consumer) {
public void disconnect(String path) {
LOG.info("Disconnected consumer for path {}", path);
Route route = routeRegistry.remove(path);
- route.remove();
+ if (route != null) {
+ route.remove();
+ }
if (routeRegistry.isEmpty()) {
Review Comment:
Right, and the fix was only half a fix — thanks.
`connect`, `disconnect`, `start`, `stop` and `isServingConsumers` are now `synchronized`, so `remove(path) → isEmpty() → stop()` is one decision and two disconnects cannot both reach `stop()`. These are lifecycle calls, not a hot path, so the monitor costs nothing that matters. Done in 86af339.
_Claude Code on behalf of oscerd_
##########
components/camel-vertx/camel-vertx-websocket/src/main/java/org/apache/camel/component/vertx/websocket/VertxWebsocketHost.java:
##########
@@ -236,6 +239,14 @@ public void stop() throws ExecutionException, InterruptedException {
port = VertxWebsocketConstants.DEFAULT_VERTX_SERVER_PORT;
}
+ /**
+ * Whether this host still serves any consumer. Every consumer bound to the same host and port shares one instance,
+ * so the host outlives the first consumer that stops, and only once the last one goes is its server stopped.
+ */
+ public boolean isServingConsumers() {
Review Comment:
Correct — `server` is now `volatile`, so a `start()` that follows a `stop()` sees the null the `finally` block wrote. The lifecycle methods are synchronized as well, so the check and the assignment no longer race either.
_Claude Code on behalf of oscerd_
##########
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;
+
+/**
+ * Consumers bound to the same host and port share one {@link VertxWebsocketHost}, so stopping one of them must leave
+ * the others able to serve and, later, to disconnect.
+ */
+public class VertxWebsocketMultiConsumerLifecycleTest extends VertxWebSocketTestSupport {
+
+ private Map<VertxWebsocketHostKey, VertxWebsocketHost> hostRegistry() {
+ return context.getComponent("vertx-websocket", VertxWebsocketComponent.class).getVertxHostRegistry();
+ }
+
+ @Test
+ void theHostOutlivesTheFirstConsumerToStop() throws Exception {
+ assertThat(hostRegistry()).hasSize(1);
+
+ context.getRouteController().stopRoute("a");
+
+ // the host still serves route b, so it must still be reachable - it used to be dropped from the
+ // registry here, which left b unable to ever disconnect and its server running for good
+ assertThat(hostRegistry()).hasSize(1);
+
+ MockEndpoint mockEndpoint = getMockEndpoint("mock:result");
+ mockEndpoint.expectedBodiesReceived("Hello b");
+
+ template.sendBody("vertx-websocket:localhost:" + port + "/test/b", "b");
+
+ mockEndpoint.assertIsSatisfied();
+ }
+
+ @Test
+ void theHostGoesWithTheLastConsumerToStop() throws Exception {
+ assertThat(hostRegistry()).hasSize(1);
+
+ context.getRouteController().stopRoute("a");
+ context.getRouteController().stopRoute("b");
+
+ assertThat(hostRegistry()).isEmpty();
+ }
+
+ @Test
+ void stoppingAConsumerTwiceIsHarmless() throws Exception {
Review Comment:
Added `consumersSharingAHostCanBeStoppedConcurrently`: both routes on the shared host are stopped from two threads released by a latch, and it asserts no failures and an empty host registry.
Worth being straight about what it proves: it exercises the concurrent path and catches an exception or a hang, but a race test cannot demonstrate the absence of a race. The guarantee comes from the monitor now held across the compound operation.
_Claude Code on behalf of oscerd_
##########
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:
Converted to `org.junit.jupiter.api.Assertions` to match the rest of the module, and the `assertj-core` test dependency goes away with it. It also removes a hunk that would have conflicted with #26541, which added the same dependency.
_Claude Code on behalf of oscerd_
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26533:
URL: https://github.com/apache/camel/pull/26533#issuecomment-5710877009
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot although they are normally detected and executed by CI.
* You can label PRs using `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
:warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #26533:
URL: https://github.com/apache/camel/pull/26533#issuecomment-5739987396
<!-- camel-backport-bot -->
### 🔄 Backport Bot
**Port PRs created:**
- ✅ `camel-4.22.x` → #26610
ℹ️ If you push additional commits after `/port`, use `/port` again to update the port PRs.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on PR #26533:
URL: https://github.com/apache/camel/pull/26533#issuecomment-5710915395
<!-- camel-backport-bot -->
### 🔄 Backport Bot
This bugfix targets `main` and may need porting to:
- `camel-4.22.x`
Labels `port/<branch>` have been added. Remove a label to opt out of porting to that branch.
Port PRs will be created automatically when this PR is merged. Comment `/port` to create them now.
ℹ️ If you push additional commits after `/port`, use `/port` again to update the port PRs.
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CAMEL-24788: camel-vertx-websocket - keep the shared host until its last consumer stops [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus merged PR #26533:
URL: https://github.com/apache/camel/pull/26533
--
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: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org