Handle LwM2M requests asynchronously

[koalatux]

Is your enhancement proposal related to a problem? Please describe.
Handling of LwM2M requests is done in a blocking way. While handling a request no other requests can be handled at that time. To build a response we would like to query information from an external component, which can take some seconds. We would like to handle other LwM2M requests during that time.

Describe the solution you'd like
In a request callback we would like to store the LwM2M request context away, send an empty CoAP ACK and tell the LwM2M engine it should not send a response upon returning of the callback. Later we want to send the response separately from a different thread using the previously stored request context.

Describe alternatives you've considered
We considered calling lwm2m_acknowledge() to send an empty CoAP ACK and then block the LwM2M thread until we have the final response. While this somewhat lowers the pain it still does not allow handling of other LwM2M messages nor does it send repeated acknowledge messages for receiving duplicate requests in case the first acknowledge got lost.

Additional context
None

[github-actions]

Hi @koalatux! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[boaks]

At least in the earlier LwM2M TS (1.0) the recommendation was to use "always an empty token", which will not work with parallel pending requests from the same LwM2M server. I guess, pretty much are using none-empty tokens, so in practice that may work. I would consider to add then some notes on that.

[rlubos]

I've finally got some time to look into this and I am wondering how to best tackle this. One idea that came to my mind is to introduce an API pair that the application could use from the registered lwm2m_engine_get_data_cb_t to inform the engine to send the reply later (similarly to how lwm2m_acknowledge() is used):

New API Proposal

/**
 * @brief Request LwM2M engine to handle current message asynchronously
 *
 * The application read handler can use this function to notify the engine that
 * this message should be handled asynchronously, for example when the request
 * data is not ready. If the request is accepted by the engine, the function will
 * return a pointer to the opaque message context and the engine will defer
 * sending the response until @ref lwm2m_send_async_response is called by the
 * application.
 *
 * @note The application takes full ownership of the message context returned
 * by this function and is responsible for calling @ref lwm2m_send_async_response
 * respectively. Not doing so will lead to message context leak in the engine.
 *
 * @param[in] client_ctx LwM2M context
 *
 * @return A pointer to the opaque message context if the message can be handled
 * asynchronously, NULL otherwise.
 */
void *lwm2m_request_async_response(struct lwm2m_ctx *client_ctx);

/**
 * @brief Send asynchronous response for the previously deferred request
 *
 * Notify the LwM2M engine to send an asynchronous response for the previously
 * stored message context. The application should call this function when all
 * data that originally led to delaying the response is ready. After calling
 * this function (regardless of the result) @ref msg_ctx should no longer by
 * used by the application.
 *
 * @note When preparing the asynchronous response the engine will call again all
 * of the read handlers for requested resources. It's the application
 * responsibility to make sure that the data can be provided by the callbacks at
 * this point. Calling @ref lwm2m_request_async_response again from the callback
 * will be treated as an internal server error and an error response will be sent
 * instead.
 *
 * @param[in] client_ctx LwM2M context
 * @param[in] msg_ctx Opaque message context returned by @ref lwm2m_request_async_response
 *
 * @return 0 on success or a negative error code in case of error.
 */
int lwm2m_send_async_response(struct lwm2m_ctx *client_ctx, void *msg_ctx);

The processing would look something like this:

1. When LwM2M READ request is received, the engine handles the request and for resources that that have a read callback registered, calls the callback.
2. From the registered lwm2m_engine_get_data_cb_t callback the application calls lwm2m_request_async_response(). The engine checks whether the current request can be handled asynchronously (for example if it has a non-empty token) and returns a pointer to the current message context if the request is accepted. The application may return NULL from the callback as the engine will ignore it.
3. The engine proceeds with handling the request but instead of sending the response it replies with empty ACK. The message context should probably be stored on some deferred requests list to acknowledge duplicates.
4. When all data on the application side is ready, it should call lwm2m_send_async_response() with the previously obtained message context. The engine will re-create the response and send it this time.

The one disadvantage I see here is that the engine will need to call all registered callbacks again when re-creating the response, so it leaves some extra logic on the application side to write the callbacks in a way they can actually provide the data after lwm2m_send_async_response() is called. But the alternatives I've considered are even worse so it's probably not that bad.

Alternatives

I've also considred introducint a new request callback type, passing the currently processed struct lwm2m_message * for each request, to allow the application whether to process the current message right now or later, but this seems to leave a lot of responsiblity on the application side (it'd need to parse the message content, check what resources are requested, this gets even trickier with composite requests) - this might be a simpler approach from the engine perspective, but due to app-related complexity I've dropped the idea.

@koalatux @boaks @SeppoTakalo @brorsensorbee @mlasch

Any thoughts, opinions on this topic? Or maybe other alternatives?

[boaks]

As I wrote above, my lwm2m time was long ago.

"Academically", defining "(for example if it has a non-empty token)" may be considered as non RFC 7252 compliant.

5.3.1. Token

An endpoint receiving a token it did not generate MUST treat the
token as opaque and make no assumptions about its content or
structure.

Also, if the device trigger a time consuming function, I would at least mention/recommend a timeout for that.

[SeppoTakalo]

@rlubos my thoughts on this.

First, I think the API conceptually looks OK. Only the application really knows if certain resources require longer processing time. If we do this, maybe lwm2m_acknowledge() should be dropped from the public API as it aims to do the same, but not proberly. Requesting a message to be handled asynchronously should automatically trigger the CoAP Ack internally and postpone further processing.

Secondly, all callbacks registered to a resource, should be able to pause the processing of the message. So the functionality should be equivalent for pre-write, validate, post-write, read, delete and execute callbacks. Any of those might require a longer processing time if they trigger an event on external system.

Thirdly, request-async and send-response pair sounds a bit off, as this postponing the processing of certain incoming request from server, until the application is ready to continue. However, withing the callback, the application (or engine) does not know the remaining content of the message. So if there is write command for multiple resources, all of them might request pause on processing. So the API that instructs the engine to continue on this message should probably be like lwm2m_continue_something(). I'm not yet sure of proper naming, but I hope you get the point. Pause-resume or postpone-continue, or similar.

Last point, If we allow any callbacks to request a processing delay, should it be and known return code for callbacks? For example, returning a negative error, is now automatically sending a CoAP error as a response and returning a zero is OK. But we could define for example that returning a positive EINPROGRESS or similar, would stop the processing of this message. Maybe easier to refactor the lwm2m_message_handling.c this way, but I don't how to deal with the given message handle then. Maybe it should be kept in the struct lwm2m_ctx * so only a one new API would be required lwm2m_continue_processing().

[rlubos]

Thanks @SeppoTakalo for the feedback, I need to rethink my initial proposal based on this - I'll investigate how viable it would be to pause/resume message processing.

First, I think the API conceptually looks OK. Only the application really knows if certain resources require longer processing time. If we do this, maybe lwm2m_acknowledge() should be dropped from the public API as it aims to do the same, but not proberly. Requesting a message to be handled asynchronously should automatically trigger the CoAP Ack internally and postpone further processing.

I personally wouldn't be so hasty on removing lwm2m_acknowledge(), it should generally be easier to use than the new mechanism, so if some app does not care about delays in message processing it might be preferable option.

Secondly, all callbacks registered to a resource, should be able to pause the processing of the message. So the functionality should be equivalent for pre-write, validate, post-write, read, delete and execute callbacks. Any of those might require a longer processing time if they trigger an event on external system.

Ok, I had initially only reading in mind, but it seems reasonable to extend this to other ops as well. Will keep that in mind.

Thirdly, request-async and send-response pair sounds a bit off, as this postponing the processing of certain incoming request from server, until the application is ready to continue. However, withing the callback, the application (or engine) does not know the remaining content of the message. So if there is write command for multiple resources, all of them might request pause on processing. So the API that instructs the engine to continue on this message should probably be like lwm2m_continue_something(). I'm not yet sure of proper naming, but I hope you get the point. Pause-resume or postpone-continue, or similar.

Yeah, I'd need to use different naming then. Originally I was thinking (with read in mind) that the engine would do a double-pass on the message, but that wouldn't be a good idea for writes for instance. I'll investigate further how viable it is to pause in the middle of processing instead.

Last point, If we allow any callbacks to request a processing delay, should it be and known return code for callbacks? For example, returning a negative error, is now automatically sending a CoAP error as a response and returning a zero is OK. But we could define for example that returning a positive EINPROGRESS or similar, would stop the processing of this message. Maybe easier to refactor the lwm2m_message_handling.c this way, but I don't how to deal with the given message handle then. Maybe it should be kept in the struct lwm2m_ctx * so only a one new API would be required lwm2m_continue_processing().

The thing is, some callback return codes (ints) and some return pointers - that's why I needed the function to "request async processing". The idea was to simply ignore the result from the callback in case processing was halted.

[SeppoTakalo]

Regarding the token discussion, which I think is not super relevant for now...however I took a look of specs.

LwM2M 1.1 specification don't really mention or require CoAP tokens, but all servers seem to use them. Otherwise would be impossible to match non piggybacked response to an earlier request, or at least it is error prone to assume and incoming response is a response to a previous request that was acknowledged earlier. Duplicate and reordered packets might cause errors.

In CoAP RFC the 5.3.1 section pretty much requires tokens, if non-piggybacked responses are expected.

In Zephyr's case, we just echo what ever token (and size) we receive. So it should be fine.

[SeppoTakalo]

The thing is, some callback return codes (ints) and some return pointers - that's why I needed the function to "request async processing".

This is true, however, I would prefer that those callbacks that are supposed to give you a memory pointer, would still be able to fail and return an error code (instead of NULL).

So maybe it is a time to refactor a bit.

And, if we refactor, maybe it is a time to change callbacks to have struct lwm2m_obj_path *path instead of one to three uint16_t values to indicate which resource instance the call is pointed to.

[rlubos]

The thing is, some callback return codes (ints) and some return pointers - that's why I needed the function to "request async processing".

This is true, however, I would prefer that those callbacks that are supposed to give you a memory pointer, would still be able to fail and return an error code (instead of NULL).

So maybe it is a time to refactor a bit.

And, if we refactor, maybe it is a time to change callbacks to have struct lwm2m_obj_path *path instead of one to three uint16_t values to indicate which resource instance the call is pointed to.

After some thinking I think we'd still need some function to request async processing, so that the application has some handle it can use to resume the processing. So far the idea was to return a pointer to the lwm2m_msg struct (which would be opaque to the app), which would then be passed to the engine by the app when resuming. Otherwise I think we'd be limited to process only a single message asynchronously.