search

Com-AugustCellars / CoAP-CSharp

CoAP Implementation in C#
Other
41 stars 19 forks source link

ProactiveCancel keeps observe alive #84

Closed trampster closed 4 years ago

trampster commented 4 years ago

CoapObserveRelation.ProactiveCancel() creates a new request to send the observe cancel. Then in the ObserveLayer in ReceiveResponse that request to cancel is not itself cancelled (IsCancelled is false) so PrepareReregistration is called which refreshes the observe that was just cancelled.

This results in the cancelled observe coming back.

trampster commented 4 years ago

Changing 

if (exchange.Request.IsCancelled) {

to 

if (exchange.Request.IsCancelled || exchange.Request.Observe == 1)

Fixes the issue for us

jimsch commented 4 years ago

File and line number would be useful

trampster commented 4 years ago

Ah yes sorry,

https://github.com/Com-AugustCellars/CoAP-CSharp/blob/master/CoAP.NET/Stack/ObserveLayer.cs

line 127

I can submit a pull request if that helps.

jimsch commented 4 years ago

Are you not hitting line 112 in Server/ServerMessageDeliverer.cs?

jimsch commented 4 years ago

By itself that does not look like it would prevent a new refresh from occurring all it does is to tell the server that the observe that it has returned as being marked canceled is to be canceled.

I am wondering why line 86 in CoapObserveRelation.cs which sets the IsCancelled to true is not lasting into this return point.

trampster commented 4 years ago

There are two Requests in CoapObserveRelation.cs ProactiveCancel(),

Line 73 makes a new request to send the cancel, this one is not marked as cancelled. Line 86 is the original observe request this is the one that is marked as cancelled.

It's the new request (line 73) that causes the problem because it is an observe and is not cancelled so it gets refreshed. My change above checks that this new 'Request to cancel' doesn't get refreshed.

jimsch commented 4 years ago

The real error is that for an Observe=1 request "The resulting response MUST NOT include an Observe Option". I think it makes sense to also mark the second request with ObserveReconnect = false so that the second condition will fail and it 1) will give the client a response to the proactive cancel and 2) not setup the reconnect.

trampster commented 4 years ago

The Resulting response does not include an Observe Option

jimsch commented 4 years ago

If you don't have have an observe option in the response, how is line 126 true?

trampster commented 4 years ago

So the plot thickens, The response MID doesn't match the request. The response is from the observe (before it was cancelled) but the request in the exchange (exchange.Request) is the cancel request. They have the same Token but different MID so should not have been put together.

When the actual Cancel response arrives it doesn't have the observe option set.

So looks like there is a bigger bug here.

jimsch commented 4 years ago

I had started to wonder if it was a race condition here. The problem is that every MID for a notification is going to be different from the original request. But the response to the cancel request should have a MID to match with. The same token is going to be used for all of them. I will need to ponder.

trampster commented 4 years ago

Thanks, can I just say that it is amazing to work with an open source project where the maintainer is so responsive.

jimsch commented 4 years ago

I think that pull request #85 should fix your problem. I am going to sit on it for a few days before merging it so that I have a chance to recover from it and see if it needs some fixes.

jimsch commented 4 years ago

Part of drop 1.10.0