{"action":"opened","issue":{"url":"https://api.github.com/repos/aws/aws-iot-device-sdk-embedded-C/issues/33","repository_url":"https://api.github.com/repos/aws/aws-iot-device-sdk-embedded-C","labels_url":"https://api.github.com/repos/aws/aws-iot-device-sdk-embedded-C/issues/33/labels{/name}","comments_url":"https://api.github.com/repos/aws/aws-iot-device-sdk-embedded-C/issues/33/comments","events_url":"https://api.github.com/repos/aws/aws-iot-device-sdk-embedded-C/issues/33/events","html_url":"https://github.com/aws/aws-iot-device-sdk-embedded-C/issues/33","id":167974379,"number":33,"title":"Disconnects and handling within library and samples","user":{"login":"tim-nordell-nimbelink","id":18471563,"avatar_url":"https://avatars.githubusercontent.com/u/18471563?v=3","gravatar_id":"","url":"https://api.github.com/users/tim-nordell-nimbelink","html_url":"https://github.com/tim-nordell-nimbelink","followers_url":"https://api.github.com/users/tim-nordell-nimbelink/followers","following_url":"https://api.github.com/users/tim-nordell-nimbelink/following{/other_user}","gists_url":"https://api.github.com/users/tim-nordell-nimbelink/gists{/gist_id}","starred_url":"https://api.github.com/users/tim-nordell-nimbelink/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tim-nordell-nimbelink/subscriptions","organizations_url":"https://api.github.com/users/tim-nordell-nimbelink/orgs","repos_url":"https://api.github.com/users/tim-nordell-nimbelink/repos","events_url":"https://api.github.com/users/tim-nordell-nimbelink/events{/privacy}","received_events_url":"https://api.github.com/users/tim-nordell-nimbelink/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2016-07-27T22:45:38Z","updated_at":"2016-07-27T22:45:38Z","closed_at":null,"body":"I've been testing the disconnect path by pointing to a custom MQTT broker and killing the broker on the server while it's connected.  With the shadow_sample, this results in the sample exiting without reconnecting despite the reconnect flag being set within the sample.  (It calls\r\naws_iot_shadow_set_autoreconnect_status()).  The output from the sample is as follows:\r\n\r\n```\r\n=======================================================================================\r\n\r\nOn Device: window state false\r\nUpdate Shadow: {\"state\":{\"reported\":{\"temperature\":31.500000,\"windowOpen\":false}}, \"clientToken\":\"c-sdk-client-id-12\"}\r\nERROR: iot_tls_write L#274  failed\r\n  ! mbedtls_ssl_write returned -0x50\r\n\r\n\r\n*****************************************************************************************\r\n\r\nERROR: main L#261 An error occurred in the loop -1\r\nDisconnecting\r\nERROR: iot_tls_write L#274  failed\r\n  ! mbedtls_ssl_write returned -0x50\r\n\r\n\r\nERROR: main L#268 Disconnect error -1\r\n```\r\nThe code path inside _aws_iot_mqtt_internal_yield() needs to handle the NETWORK_DISCONNECTED_ERROR error for the aws_iot_mqtt_internal_cycle_read() call, too, which is why the example application does not reconnect.  The suggested change for this point is to do:\r\n\r\n```diff\r\n                }\r\n \r\n                yieldRc = aws_iot_mqtt_internal_cycle_read(pClient, &timer, &packet_type);\r\n-               if(SUCCESS != yieldRc) {\r\n-                       break;\r\n-               }\r\n \r\n-               yieldRc = _aws_iot_mqtt_keep_alive(pClient);\r\n+               if(SUCCESS == yieldRc)\r\n+                       yieldRc = _aws_iot_mqtt_keep_alive(pClient);\r\n+\r\n```\r\nwhich would pass down the  NETWORK_DISCONNECTED_ERROR down from the aws_iot_mqtt_internal_cycle_read() call into the handling block for this.\r\n\r\nAfter making this change, however, this exposes that the code below doesn't state transition to disconnected before the client set state call just below.  To fix this, I've changed the above to:\r\n\r\n```diff\r\ndiff --git a/src/aws_iot_mqtt_client_yield.c b/src/aws_iot_mqtt_client_yield.c\r\nindex b86a785..e80916d 100644\r\n--- a/src/aws_iot_mqtt_client_yield.c\r\n+++ b/src/aws_iot_mqtt_client_yield.c\r\n@@ -201,12 +201,13 @@ static IoT_Error_t _aws_iot_mqtt_internal_yield(AWS_IoT_Client *pClient, uint32_\r\n                }\r\n \r\n                yieldRc = aws_iot_mqtt_internal_cycle_read(pClient, &timer, &packet_type);\r\n-               if(SUCCESS != yieldRc) {\r\n-                       break;\r\n-               }\r\n \r\n-               yieldRc = _aws_iot_mqtt_keep_alive(pClient);\r\n+               if(SUCCESS == yieldRc)\r\n+                       yieldRc = _aws_iot_mqtt_keep_alive(pClient);\r\n+\r\n                if(NETWORK_DISCONNECTED_ERROR == yieldRc) {\r\n+                       if(clientState != CLIENT_STATE_DISCONNECTED_ERROR)\r\n+                               _aws_iot_mqtt_handle_disconnect(pClient);\r\n                        pClient->clientData.counterNetworkDisconnected++;\r\n                        if(1 == pClient->clientStatus.isAutoReconnectEnabled) {\r\n                                yieldRc = aws_iot_mqtt_set_client_state(pClient, CLIENT_STATE_DISCONNECTED_ERROR,\r\n```\r\nI'm still digging through the issues in the codebase around the reconnects to understand how the codebase is supposed to fit together, but thought I'd open this issue as it's quite evident with the shadow_sample and closing the connection from the server side.\r\n\r\nThe above doesn't work by itself as it exposes the following issue with the codebase, too, relating to reconnecting:\r\n\r\nThe function call path:\r\n```\r\n -> _aws_iot_mqtt_handle_disconnect()\r\n -> aws_iot_mqtt_disconnect()\r\n -> _aws_iot_mqtt_internal_disconnect\r\n -> pClient->networkStack.destroy() (which points to iot_tls_destroy())\r\n```\r\nis problematic since the only corresponding iot_tls_init() call is within the aws_iot_mqtt_init() call.  An example problem point is within _aws_iot_mqtt_keep_alive() which invokes this path, and returns expecting the caller to attempt to reconnect, but technically the caller will be operating on a destroy'd handle.)\r\n"}}
{"actor":{"display_login":"tim-nordell-nimbelink"}}