allthingssecurity commented on code in PR #28069:
URL: https://github.com/apache/camel/pull/28069#discussion_r4311053151
##########
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:
Thanks for this. Your reasoning holds for the cancel: with `status` set to
STOPPING before `doStop()` and the volatile fields, either `doStop()` cancels
the new observation with its relation set, or the re-check sees that the
consumer is stopping and cancels it.
I went with the lock in 91d6046330f1 though, because of the client side.
With the re-check, `doStop()` can run completely, including
`client.shutdown()`, between reading `client` and `current.observe(next)`. In
Californium 3.14.0 `shutdown()` only nulls and `shutdownNow()`s the two
executors, and `observe()` then calls `getSecondaryExecutor()`, which lazily
creates a new `CoapClient(secondary)` executor that nothing shuts down. So the
observation is cancelled, but each such race leaves an executor behind.
The new test `observerStoppedWhileObservingAgainCancelsTheNewObservation`
stops the consumer from inside the observe call of the observe-again (no
production hook: the endpoint returns a `CoapClient` subclass). On the previous
commit it fails on the leaked observation; with the re-check instead of the
lock it fails on `observed again with the client that was shut down`; with the
lock it passes. `doStart`, `doStop` and the observe-again in `onError` take one
`ReentrantLock` (as `CoAPComponent` / `CoAPProducer` use); a cancelled handler
drops responses without taking it, so the cancel inside `doStop` cannot
deadlock with a late response. camel-coap: 82 tests, 0 failures.
_Claude Code on behalf of allthingssecurity_
##########
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:
Thanks, both windows are real. Closed in 91d6046330f1: `doStart`, `doStop`
and the observe-again in `onError` now hold one lock (a `ReentrantLock`, so
`onError` -> `startObserve` is fine), and `onError` observes again only while
the consumer is running and still has its client. New test:
`observerStoppedWhileObservingAgainCancelsTheNewObservation`, which fails on
the previous commit (the new observation is not cancelled and the server keeps
it).
_Claude Code on behalf of allthingssecurity_
##########
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:
I kept it on purpose: `CoAPObserver` is a public class and `onLoad` /
`onError` are public, so removing the interface would be an API change for
anyone who calls or extends them. `Observation` delegates to those methods, so
they stay the one place where a response is handled. Claus is fine with either.
_Claude Code on behalf of allthingssecurity_
--
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]
