LwM2mObservationStore 'remove(Token token)' is not functionnal

[cyril2maq]

Version(s)

fa38111

Which components

leshan server

Tested With

No response

What happened

LwM2mObservationStore.remove(Token token) calls registrationStore.removeObservation(String registrationId, ObservationIdentifier observationId) : https://github.com/eclipse/leshan/blob/fa38111cf1d4787ead04bd04002c2df7cccc8dc1/leshan-server-cf/src/main/java/org/eclipse/leshan/server/californium/observation/LwM2mObservationStore.java#L95-L96

But it does call it with 'null' registrationId.

And RedisRegistrationStore 'needs' a non null registrationId as it uses it as lock; and does nothing if it does not exist :
https://github.com/eclipse/leshan/blob/fa38111cf1d4787ead04bd04002c2df7cccc8dc1/leshan-server-redis/src/main/java/org/eclipse/leshan/server/redis/RedisRegistrationStore.java#L557-L564

May be the bug is more on the RedisRegistrationStore side who could allow a null registrationId and still remove the observationId.

How to reproduce

Create a 'bad' Observation (e.g. unknown path) with a server using RedisRegistrationStore : the observation is not deleted from the store.

In this use case, the BaseMatcher.ObservationObserverAdapter.onResponse callback from californium library will call the LwM2mObservationStore.remove(Token token) method (due to the if (response.isError()...) l.332) wich will trigger no deletion as explained.

As a consequence, the Observation will still be registered in the store whereas it should have been deleted.

Relevant Output

No response

[sbernard31]

Thx for reporting this. 🙏

I take a look at this quickly and this smells not so good, you probably find a bug in RedisRegistrationStore.
The abstraction for the observation function is clearly the one I am least satisfied with... but didn't find anything better ...

I will first try to see if we have a unit test which should cover this.
If not I will add a new one.
Then I will try to fix the bug and propose a PR.

[sbernard31]

Ok we have test for this :
https://github.com/eclipse/leshan/blob/a6e55d7370271e6405ffbdd8291cec6450b976f5/leshan-integration-tests/src/test/java/org/eclipse/leshan/integration/tests/observe/ObserveTest.java#L153-L201

(which is tested with Redis via RedisObserveTest)

It use server.getObservationService().cancelObservation(observation); and is ✔️

When you say :

Create then cancel an Observation with a server using RedisRegistrationStore : the observation is not deleted from the store.

I guess this is related to :
https://github.com/eclipse/leshan/blob/a6e55d7370271e6405ffbdd8291cec6450b976f5/leshan-server-cf/src/main/java/org/eclipse/leshan/server/californium/observation/LwM2mObservationStore.java#L86-L91
which is called by californium.

But even if server.getObservationService().cancelObservation(observation); trigger the californium call, it also removes Observation from the store first, So I'm not sure how you get "the observation is not deleted from the store." 🤔

See : https://github.com/eclipse/leshan/blob/a6e55d7370271e6405ffbdd8291cec6450b976f5/leshan-server-core/src/main/java/org/eclipse/leshan/server/observation/ObservationServiceImpl.java#L123-L130

I'm curious to know how you reproduce it exactly. 😁

(Note that I also see the multiple call issue reported by #1385)

Anyway, I will try to propose a fix for RedisRegistrationStore about null registration id.

[cyril2maq]

Thanks again for your analysis !
I apologize, I did mix up my use cases when trying this new version. The 'How to reproduce' is not the right one 😣
I will update my original post.

[sbernard31]

I created a PR about this #1388.

@cyril2maq if you want you can take a look at it. (better to read it commit by commit I guess)

[sbernard31]

This is not integrated in master 🙂