Client subscription on change doesn't work

[pstolarz]

Another issue from my side. Now the problem is related to client's subscription on a change.

I have my own object /1001/0 with a single resource 0 of type boolean and a subscription to a change of that resource via local client's C code as follows:

// subscribe to a change
AwaClientSubscribeOperation *subscribeOperation =
    AwaClientSubscribeOperation_New(session);

AwaClientChangeSubscription *subscription =
    AwaClientChangeSubscription_New("/1001/0/0", heaterOn_cb, NULL);
AwaClientSubscribeOperation_AddChangeSubscription(
    subscribeOperation, subscription);
AwaClientSubscribeOperation_Perform(
    subscribeOperation, OPERATION_PERFORM_TIMEOUT);

NOTE: The object and the resource are not mandatory, therefore are created at the local client startup:

AwaClientSetOperation_CreateObjectInstance(operation, "/1001/0");
AwaClientSetOperation_CreateOptionalResource(operation, "/1001/0/1");
AwaClientSetOperation_AddValueAsBoolean(operation, "/1001/0/0", heaterOn);

On the previous version of the library (before the fix of my previous bug #269 with BOOTSTRAP WRITE) it worked fine (client was notified via subscription), now (WRITE via CoAP PUT) it doesn't work.

After some investigation I found the root cause in lwm2m_client_core.c and the CoAP PUT handler HandlePutRequest():

switch (Lwm2mTreeNode_GetType(root))
{
    case Lwm2mTreeNodeType_Object:
        Lwm2m_Error("Cannot replace a whole object using PUT\n");
        *responseCode = AwaResult_MethodNotAllowed;
        break;
    case Lwm2mTreeNodeType_ObjectInstance:
        if ((*responseCode = Lwm2mCore_CheckWritePermissionsForObjectInstanceNode(context, origin, root, oir[0], false)) == AwaResult_Success)
        {
            if (Lwm2mCore_Exists(context, oir[0], oir[1], oir[2]) && Lwm2mCore_Delete(context, origin, oir[0], oir[1], oir[2], true) == AwaResult_SuccessDeleted)
            {
                *responseCode = Lwm2mCore_ParseObjectInstanceNodeAndWriteToStore(context, root, oir[0], true, true, false, &oir[1]);
            }
            else
            {
                *responseCode = AwaResult_NotFound;
            }
        }
        break;
    case Lwm2mTreeNodeType_Resource:
        if ((*responseCode = Lwm2mCore_CheckWritePermissionsForResourceNode(context, origin, root, oir[0], oir[1], false)) == AwaResult_Success)
        {
            if (Lwm2mCore_Exists(context, oir[0], oir[1], oir[2]) && Lwm2mCore_Delete(context, origin, oir[0], oir[1], oir[2], true) == AwaResult_SuccessDeleted)
            {
                *responseCode = Lwm2mCore_ParseResourceNodeAndWriteToStore(context, root, oir[0], oir[1], true);
            }
            else
            {
                *responseCode = AwaResult_NotFound;
            }
        }
        break;
    case Lwm2mTreeNodeType_ResourceInstance: // no break
    default:
        // Should never happen.
        Lwm2m_Error("Internal Error\n");
        *responseCode = AwaResult_InternalError;
        break;
}

The root cause is calling Lwm2mCore_Delete() before writing (replacing) object instance OR resource instance.

I don't know the rationale of such implementation for PUT method handling, but such approach will cause object/resource re-creation via Lwm2mCore_ParseObjectInstanceNodeAndWriteToStore() / Lwm2mCore_ParseResourceNodeAndWriteToStore() call for every time on the object instance/resource change. This effectively leads to missing the object/resource change by the registered observer(s). See Lwm2m_MarkObserversChanged() while comparing the new and old values.

On the previous version of the lib (tag 0.2.3) it worked fine since for BOOTSTRAP WRITE (HandleBootstrapPutRequest() handler), the deletion was not called - bootstrapping assumes persistent objects existence.

NOTE: The root cause described here also causes the observer(s) are not called when staring a client with a factory bootstrap file (not via the BS server)!

After removing Lwm2mCore_Delete() calls in HandlePutRequest() everything works fine.

UPDATE: The delete procedure was introduced by the commit 5988c12025d88a6d7ad3ef7ba86b7a4a9d864a0d: "Fixed PUT / POST handling on multiple instance and mandatory resources.", so there is probably some rationale for doing this related to multi-instance handling. BTW as I previously mentioned - my object was not mandatory.

[ghost]

Thanks for the detailed bug report!

Yes - without delete PUT will not do a proper replace, it will only update the resources that are in the request's payload. So resources that no longer should exist for the affected object instance will still be there, and multiple instance resources will be updated rather than replaced.

I agree however we need a better solution for this - I was thinking rather than deleting the object instance (breaking all observations on the object instance and all its resources), we delete the resources under it that are not part of the PUT request, and clear any multiple-instance resources that are part of the PUT request (otherwise multiple instance resources will be updated rather than replaced)

[DavidAntliff]

Yes, it's a PUT = replace issue I think. It does sound like it needs a bit more work. The problem is that with a replace, missing resources should probably be deleted, otherwise it's a "partial replace" which is not much different to a "partial update".

[DavidAntliff]

Would be a good issue for @datachi7d to think about.

[delmet]

I also think we need to separate the logic here. As resource replace is different from object replace.

For object replace I agree with the other comments above. As you need to make sure any resources that are not specified are default with.

But for a resource replace is just basically a case of saying the value of resource is now what you are passing. Which to be honest is only really a change from POST (update) for multi instance resources.

[delmet]

I have improve replace on resources see #273, but we will still need improve object replace.

[delmet]

Sorry have trouble with tests and get resource instances back in correct order.

[pstolarz]

Thanks for comments and clarifications, but I think the discussion went in somehow wrong direction - if the current implementation of PUT/POST is proper or should be improved. I believe it's fine and there is nothing wrong in deleting and re-creating a resource (object) in case of its replacement.

My main concern in this issue was subscription handling and I'd rather think how to improve this area. Please consider my proposal, what I'm missing the most in the current subscription service (and what would resolve this and possible future issues of that kind). I will concentrate on resources since it's my main point of interest, but most of the notes below could probably apply to objects either:

• Lack of subscription on a resource deletion. If the library could inform than the resource of interest is removed, it would be easy on the user level to distinguish a replace from an update. Not mentioning, w/o this notification a library user maintains resource(s) which may have no more sense (not exist).
• In my opinion notification on a resource value change provides unnecessary complication of the library (which need to track resource values) w/o additional benefits. I would rather prefer, the library would inform me about all resource values the server sends and it's easy to track value changes (if necessary) on the user level. Additionally, in the current version of the lib a user will not be notified if a resource is set still on the same value. E.g. command-like resource, which when written with some value, will perform some action on a device like clear its logs.
• Lack of subscription on a resource creation - may be useful but not necessary.

From the above I believe, binding an observer with a resource instance is rather wrong decision. It would probably be better to associate it with a resource definition. But... it's only an implementation issue.

[DavidAntliff]

FWIW, I don't think the delete should be removed. The original design allows for the client to discriminate between a partial update and a replace, as information is contained in that distinction. The API supports change notifications allowing this distinction to be visible via the API. There is a delete change in the change notification - although I can't be sure it has been fully implemented yet. Creation should be supported too. If not, this is the area that should probably be fixed. @rolandbewick-img could examine the situation in more detail if he has time/interest.

@pstolarz If you just want write notification, use the Static API, as the full API is intended to maintain state for the user app.

Binding subscription to instance rather than definition is a long-accepted shortcoming and something that ought to be fixed at some stage.

[ghost]

@DavidAntliff you're correct - Subscription change notification types haven't been implemented yet. There'll have to be some planning around this as the LWM2M spec is not clear how to send different change types through the coap layer in a notify packet (eg. resource created / modified / deleted). Implementing the Gateway API and Core IPC layer functionality should be simple enough though.