Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async responses for Direct Methods #2570

Closed
oscarh opened this issue Jan 31, 2024 · 11 comments
Closed

Async responses for Direct Methods #2570

oscarh opened this issue Jan 31, 2024 · 11 comments
Assignees

Comments

@oscarh
Copy link

oscarh commented Jan 31, 2024

When receiving a Direct Method to the Azure IoT C SDK, the current API forces the SDK callback to deliver the response synchronously. At the same time there is documentation saying that "Your callback should in general limit itself to quick, CPU bound operations" [1]. This does make Direct Methods rather useless, if the method need to do some more work than just copy data as per your sample code. In our case, we want to pass Direct Methods to sub-systems and return the response when that sub-system has replied. We cannot block the whole SDK while waiting for those responses.

There was an API, IoTHubClient_SetDeviceMethodCallback_Ex in the (now deprecated) iothub_client/inc/iothub_client.h. I guess we are discouraged to use this API.

This question seems to be answered in quite a few places, but there is no clear answer from your side. For instance #1647 and #1437. #1647 was closed, since the work should be done in #1437, but that is closed without any good explanation. There is a discussion here
#1984, with a comment from 2021 stating "Will update you soon." Not sure how soon that is?

[1] - https://github.com/Azure/azure-iot-sdk-c/blob/main/doc/threading_notes.md#avoid-long-running-callbacks

Update: I refer to the multi-threaded functions above, but wanted to talk mainly about:
IoTHubClient_LL_SetDeviceMethodCallback_Ex and IoTHubDeviceClient_LL_SetDeviceMethodCallback

@ericwolz
Copy link
Contributor

IoTHubDeviceClient_LL_SetDeviceMethodCallback
The LL (low-level) APIs are not multithreaded, so any callback will block the SDK pipeline including MQTT communication. Blocking this for a long time will cause MQTT PUBACK and PINGREQ timeouts.

IoTHubDeviceClient_SetDeviceMethodCallback
The non LL APis uses the multithreaded SDK. All callbacks are stored on a queue and notification are sent to the application on another thread that is not part of the SDK pipeline.

@oscarh
Copy link
Author

oscarh commented Feb 1, 2024

@ericwol-msft thank you for the reply. We are uing the LL API and understand this limitation. The problem is that the IOTHUB_CLIENT_DEVICE_METHOD_CALLBACK_ASYNC callback registered in IoTHubDeviceClient_LL_SetDeviceMethodCallback MUST deliver a repsonse, or else the SDK will consider the direct method as failed iothub_client/src/iothub_client_core_ll.c:

                if (payload_resp != NULL && response_size > 0)
                {
                    result = handleData->IoTHubTransport_DeviceMethod_Response(handleData->deviceHandle, response_id, payload_resp, response_size, result);
                }
                else
                {
                    result = MU_FAILURE;
                }

@oscarh
Copy link
Author

oscarh commented Feb 1, 2024

Regarding multithreaded API. Is the queue handling synchronous, meaning that one blocking direct method would block other? In this case, this would also be a big problem for users of that API.

@oscarh
Copy link
Author

oscarh commented Feb 1, 2024

I see that I referred to the wrong functions above. The function I wanted to use is IoTHubClient_LL_SetDeviceMethodCallback_Ex which is deprecated:

IoTHubClient_LL_SetDeviceMethodCallback_Ex is deprecated. Use IoTHubDeviceClient_LL_SetDeviceMethodCallback() instead.

@ericwolz
Copy link
Contributor

ericwolz commented Feb 5, 2024

Regarding multithreaded API. Is the queue handling synchronous, meaning that one blocking direct method would block other? In this case, this would also be a big problem for users of that API.

Yes, there is only one worker thread for the callbacks, The callback blocks other notifications.

@ewertons
Copy link
Contributor

ewertons commented Feb 6, 2024

Hi @oscarh ,
thanks for your question.
Just to make it clear, functions or headers that have been marked as deprecated MUST not be used.

Regarding your question about direct methods, this feature is implemented to have a short life-cycle between request and response. thus the reason to have the reply at the return of the request callback. This is for optimization of resources and scalability on the server side.

If you do have a command that must take a longer time to execute and then report a status, you must implement the request as a begin and end design. First fire a direct method requesting an command to be started in your client (e.g., my_command_begin) and then after a certain time (estimated by you in your implementation), send another direct method requesting either a status or the final report of the execution (e.g., my_command_end).

@ewertons ewertons self-assigned this Feb 6, 2024
@oscarh
Copy link
Author

oscarh commented Feb 7, 2024

Hi @ewertons,

Thanks for you reply.

I'm afraid your comment doesn't make much sense though, since the REST API for calling a direct method has a responseTimeoutInSeconds which can be between 5 and 300 seconds. [1]. I hope we're not expecting the IoT Hub to add 300 seconds latency? Apart from that, there is a connectTimeoutInSeconds which can also be up to 300 seconds. During the second timeout the hub will wait for an offline device to come online. If the backend can handle this in a scalable way, it dosen't make sense that the device must only do "quick, CPU bound operations".

Furthermore, if I would try to implement your suggestion with a start and and end part, the end part would need to poll the device, using direct methods, since I cannot know when the response is ready. It would probably consume more resources everywhere.

Actually the whole things feels like something went wrong during a refactoring, why would otherwise the type for the callback be named IOTHUB_CLIENT_DEVICE_METHOD_CALLBACK_ASYNC although it cannot do any async operation?

[1] - https://learn.microsoft.com/en-us/azure/iot-hub/iot-hub-devguide-direct-methods#method-invocation

@ewertons
Copy link
Contributor

Hi @oscarh ,

Unfortunately the Azure IoT C SDK does not implement a way to receive direct methods requests and provide responses independently.

The options I shared are alternatives given the current implementation and limitations of the Azure IoT C SDK.
If you are looking for more granular control of an SDK for Azure IoT Hub, please take a look at Azure SDK for C, which is also more suited for embedded devices.

@oscarh
Copy link
Author

oscarh commented Feb 21, 2024

HI @ewertons ,

But it did support it until the function IoTHubClient_SetDeviceMethodCallback_Ex in header iothub_client/inc/iothub_client.h was deprecated in commit e3e0a0b on Tue Oct 27 08:46:40 2020 -0600.

@ewertons
Copy link
Contributor

ewertons commented Mar 6, 2024

That seems correct. At this time, we do not plan to implement this enhancement in the currently supported API headers to match the behavior of the deprecated header you mentioned.

@ewertons
Copy link
Contributor

Closing this issue based on the latest triage decision.
Thanks,
Azure IoT SDK Team

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants