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

Stop client before destroying #547

Merged
merged 7 commits into from
Jul 31, 2024
Merged

Stop client before destroying #547

merged 7 commits into from
Jul 31, 2024

Conversation

szczys
Copy link
Contributor

@szczys szczys commented Jul 30, 2024

  • Fix memory free for post_block requests in both libcoap and Zephyr coap
  • Stop client before destroying it in both libcoap and Zephyr coap
  • libcoap: call golioth_sys_client_disconnected() when ending a session
  • libcoap: disable automatic cloud logging when the client is disconnected

resolves https://github.com/golioth/firmware-issue-tracker/issues/584

szczys added 2 commits July 30, 2024 11:59
When purge_request_mbox() it was conditionally freeing POST quests but
should also check for POST_BLOCK requests which have a dynamically
allocated payload.

Signed-off-by: Mike Szczys <[email protected]>
If the client is running when golioth_client_destroy() is called it needs
to be stopped before freeing the client.

When the client is stopped, the logging backend will be disabled,
preventing log messages from trying to access the client after it is freed.
The linked list of requests (client->coap_reqs) will then also be freed as
part of the stop process, ensuring there isn't any allocated OBSERVE or
POST/POST_BLOCK memory leaked.

Signed-off-by: Mike Szczys <[email protected]>
@szczys szczys requested review from mniestroj and sam-golioth and removed request for mniestroj and sam-golioth July 30, 2024 17:00
Copy link

github-actions bot commented Jul 30, 2024

Visit the preview URL for this PR (updated for commit 94fc673):

https://golioth-firmware-sdk-doxygen-dev--pr547-szczys-fix-cli-fte3fpvq.web.app

(expires Wed, 07 Aug 2024 14:56:17 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: a9993e61697a3983f3479e468bcb0b616f9a0578

@szczys szczys marked this pull request as draft July 30, 2024 17:01
Copy link

github-actions bot commented Jul 30, 2024

Code Coverage

Type Coverage
lines 58.2% (1306 of 2243 lines)
functions 66.5% (129 of 194 functions)

szczys added 2 commits July 30, 2024 13:17
When purge_request_mbox() it was conditionally freeing POST quests but
should also check for POST_BLOCK requests which have a dynamically
allocated payload.

Signed-off-by: Mike Szczys <[email protected]>
Automatic formatting was applied to this file, no substantive changes were
made to the code.

Signed-off-by: Mike Szczys <[email protected]>
@szczys szczys force-pushed the szczys/fix_client_destroy branch from 63669b9 to a0a7bf5 Compare July 30, 2024 19:50
szczys added 2 commits July 30, 2024 15:35
This prevents attempts to send logs messages to the cloud when the client
is disconnected. Upon reconnect, backend logging will be enabled based on
CONFIG_GOLIOTH_AUTO_LOG_TO_CLOUD.

Signed-off-by: Mike Szczys <[email protected]>
If the client is running when golioth_client_destroy() is called it needs
to be stopped before freeing the client.

When the client is stopped, golioth_sys_client_disconnected() is now called
which in turn sets golioth_debug_set_cloud_log_enabled() to false. This
prevents attempts to send log messages to Golioth when the client is not
connected.

Signed-off-by: Mike Szczys <[email protected]>
@szczys szczys force-pushed the szczys/fix_client_destroy branch from e7e2dcc to 63d2136 Compare July 30, 2024 20:35
@szczys szczys marked this pull request as ready for review July 30, 2024 20:36
@szczys szczys requested review from sam-golioth and mniestroj July 30, 2024 20:36
@szczys szczys force-pushed the szczys/fix_client_destroy branch from 63d2136 to 845f68e Compare July 31, 2024 14:52
- Add golioth_client_destroy() and subsequent reconnect to test ability to
  destroy and restart the client
- Add a semaphore to block until each new connection is established.

Signed-off-by: Mike Szczys <[email protected]>
@szczys szczys force-pushed the szczys/fix_client_destroy branch from 845f68e to 94fc673 Compare July 31, 2024 14:55
Copy link
Contributor

@sam-golioth sam-golioth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @szczys!

@@ -14,38 +14,45 @@
* Time
*------------------------------------------------*/

uint64_t golioth_sys_now_ms() {
uint64_t golioth_sys_now_ms()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are moving from the FreeRTOS port to the system ports (ESP-IDF, ModusToolBox, etc.) in #486, but I think it's fine to keep it here for now.

@szczys szczys merged commit 64ee83d into main Jul 31, 2024
38 of 44 checks passed
@szczys szczys deleted the szczys/fix_client_destroy branch July 31, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants