Fixed a race condition while adding and cancelling an observation

[balsmn]

This PR fixes a race condition in CaliforniumObservationRegistryImpl while invoking addObservation and cancelObservation calls. Due to this race condition it could still be possible to have a duplicate observation for the same resource and client reported in #163

Due to change that came in as a bug fix for issue #168 there can be at most only one active observation for a given resource. So the ObservationRegistry interface was changed to reflect the same.

Signed-off-by: Bala Azhagappan balasubramanian.azhagappan@bosch-si.com

[jvermillard]

Why us a Lock and not a synchronize(this) ?

[balsmn]

notifying Observation listeners is outside the locks. At the end the same can also be achieved using synchronized blocks. Its just a religious thing which one you chose.

[DanielMaier-BSI]

I think it is better to use a lock that is private to the class. Otherwise someone else could also synchronize on "this" and we might get a deadlock (more a theoretical and style discussion).

Of course we can also synchronize on a private object and use synchronized blocks.

[jvermillard]

There is no good reason to use a lock here, you can use a private monitor. "private Object monitor = new Object();" if you really want to isolate it.

A synchronize on monitor is superior here because you don't have to rely on the try/finally and you are not using the extra feature of Lock missing from synchronize block.

That's no religious/style thing, just better idiomatic java 😄

[DanielMaier-BSI]

This is what I meant with the last sentence. We can use a private monitor.

[balsmn]

@jvermillard as you suggested, I have pushed the updated version with the two changes.

[sbernard31]

Manu is modifying this code and your PR will not be so relevant in the new code.

But finally this constraint of duplicate observation should not be handled at Leshan level. This is a CoAP constraint so this should be done in californium.

[balsmn]

I agree with you regarding only one observation per resource restriction belonging to californium. The fact that we have an ObservationStore in Californium layer and an ObservationRegistry in Leshan leads to replicate the californium restriction to Leshan layer.

Could you give a heads up what kind of changes are to be expected in the new code? Is it only reimplementing ObservationRegistry or we would not have an ObservationRegistry?

[sbernard31]

The Observe CoAP spec §3.1 say:
"A client MUST aggregate such requests and MUST NOT register more than once for the same target resource. The target resource is identified by all options in the request that are part of the Cache-Key. This includes, for example, the full request URI and the Accept Option."

Currently in californium, there is no limitation about that. An obversation is just identifying by a Token.

The spec say that a target resource is identified by all the options §5.10 that are part of the cache key (= all options except size1). But, as we deviate a little from the spec by allowing observation from different endpoint (This is the case where the CoAP server is behind a NAT), Using the Cache-Key is not so relevant...

So I'm not so sure pushing this restriction in californium is still a good idea...

Could you give a heads up what kind of changes are to be expected in the new code? Is it only reimplementing ObservationRegistry or we would not have an ObservationRegistry?

Manu will probably push a PR before the end of the week.

[sbernard31]

So Manu pushed its code in the observe_context branch which needs this additionnal feature in Californium.

Unfortunately, this will not fix the race condition you find here.
A possible solution could be to let the ObservationStore removing old observation(s?) as it is in charge to keep the store in a consistent state. Ideally, it should notify Californium that the request is cancelled.

There is another race condition between observe and registration, but we think we found a miss-conception in Leshan. As observation is strongly linked to the registration, we think that observation should be store in ClientRegistry(RegistrationStore)

What do you think ?

[balsmn]

Yes I agree that Observation is strongly linked to a registration.

we think that observation should be store in ClientRegistry(RegistrationStore)

You mean to say that instead of having a separate ObservationRegistry, just have the RegistrationStore implement the CaliforniumObservationRegistry or even the new LwM2mObservationStore interface introduced in the branch (observe_context) and clear the observations when a de-registration happens?

[balsmn]

Unfortunately, this will not fix the race condition you find here.
A possible solution could be to let the ObservationStore removing old observation(s?) as it is in charge to keep the store in a consistent state. Ideally, it should notify Californium that the request is cancelled.

I agree. If InMemoryObservationStore while adding, can also cancel any existing observations. That means, the add(Observation) of ObservationStore should return list of observations removed/canceled while adding. I see that you have already done this in your Californium-PR#87

When it is implemented that way and merged in to master, then this PR will not be relevant any more, if not it would still be good to have this PR.

I have a small request. As the refactoring done in CaliforniumObservationRegistry as part of the observe-request-context has nothing to do with the observe request's extention with a context info, it would be good to separate this refactoring from the actual observe request context implementations when creating PR's. What do you think ?

[sbernard31]

You mean to say that instead of having a separate ObservationRegistry, just have the RegistrationStore implement the CaliforniumObservationRegistry or even the new LwM2mObservationStore interface introduced in the branch (observe_context) and clear the observations when a de-registration happens?

Something like this but I need to think more about this.

[msangoi]

I have a small request. As the refactoring done in CaliforniumObservationRegistry as part of the observe-request-context has nothing to do with the observe request's extention with a context info, it would be good to separate this refactoring from the actual observe request context implementations when creating PR's. What do you think ?

I'm not sure to understand why it is needed to separate this. I agree that the registry refactoring could have been done in a separate PR, but then the code handling the observe context would have been duplicated.

[sbernard31]

Manu changes was integrated in master so the PR si no more relevant.
I open the issue to track that this race condition is still present. #176