gnodet-bot commented on code in PR #28069:
URL: https://github.com/apache/camel/pull/28069#discussion_r4310232874
##########
components/camel-coap/src/main/java/org/apache/camel/coap/CoAPObserver.java:
##########
@@ -31,6 +32,7 @@
public class CoAPObserver extends DefaultConsumer implements CoapHandler {
Review Comment:
⚠️ **Stale interface declaration**: `CoAPObserver` still `implements
CoapHandler` but `client.observe()` now receives the inner `Observation`
instance, not `this`. The outer class is no longer used as a handler — remove
the interface to avoid confusion.
```suggestion
public class CoAPObserver extends DefaultConsumer {
```
##########
components/camel-coap/src/main/java/org/apache/camel/coap/CoAPObserver.java:
##########
@@ -63,13 +65,67 @@ public void onLoad(CoapResponse response) {
}
}
+ @Override
+ protected void doStop() throws Exception {
+ // cancel the observation, or the server keeps sending notifications
to this consumer after it was stopped, and
+ // a route that is started again then gets every notification twice
(once from the stopped consumer)
+ Observation current = observation;
+ observation = null;
+ if (current != null) {
+ current.cancel();
+ }
+ if (client != null) {
+ client.shutdown();
+ client = null;
+ }
+ super.doStop();
+ }
+
@Override
public void onError() {
getExceptionHandler().handleException(new IOException("CoAP request
timed out or has been rejected by the server"));
- startObserve();
+ if (isRunAllowed()) {
+ startObserve();
+ }
}
private void startObserve() {
- this.client.observe(this);
+ Observation next = new Observation();
+ this.observation = next;
+ next.relation = this.client.observe(next);
Review Comment:
⚠️ **Race condition — server-side observer leak**: `doStop()` can interleave
between `this.observation = next` (line 94) and `next.relation =
this.client.observe(next)` (line 95). When it does, `cancel()` sees `relation
== null`, skips `proactiveCancel()`, and the server retains the leaked observer
permanently.
Additionally, `doStop()` can null `client` between `isRunAllowed()` (in
`onError`) and `this.client.observe(next)` here, causing a NPE.
Both windows require a shared lock (see review body for the full fix).
--
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]
