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

Fix OTA_Shutdown() not releasing resources #1923

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vishwamartur
Copy link

Related to #1910

Modify otaThread function in demos/ota/ota_demo_core_http/ota_demo_core_http.c to release resources properly.

  • Add pthread_detach(pthread_self()); to detach the thread.
  • Add pthread_exit(NULL); to exit the thread.

Related to aws#1910

Modify `otaThread` function in `demos/ota/ota_demo_core_http/ota_demo_core_http.c` to release resources properly.

* Add `pthread_detach(pthread_self());` to detach the thread.
* Add `pthread_exit(NULL);` to exit the thread.
@vishwamartur vishwamartur requested a review from a team as a code owner October 30, 2024 12:20
@ActoryOu
Copy link
Member

ActoryOu commented Nov 1, 2024

Hello @vishwamartur,
Thanks for your contribution! Could you help share your verification result with us as well?

@aggarg
Copy link
Member

aggarg commented Nov 4, 2024

@vishwamartur Please describe how you tested these changes?

Copy link
Member

@AniruddhaKanhere AniruddhaKanhere left a comment

Choose a reason for hiding this comment

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

Thank you @vishwamartur for this contribution.

I think there might be a race condition. What if detach is called before the line 2117 (pthread_join)? That will cause undefined behavior.

And I think when the thread exits, it automatically cleans the resources. We need not have to call kill - it is an extreme measure in my opinion.

Also, I am not sure why would we need both join and detach. Both of them are mutually exclusive.

Or maybe I failed to understand something. Can you please clarify?

@vishwamartur
Copy link
Author

Hi @AniruddhaKanhere,

Thank you for highlighting the potential issues with pthread_detach and pthread_exit.

Since pthread_detach and pthread_exit are both being used, there is indeed a redundancy here. Calling pthread_exit will allow the thread to terminate and release resources, so adding pthread_detach isn't necessary and might even lead to undefined behaviour if the system tries to manage the thread's lifecycle in two ways.

I’ll update the code to use only pthread_exit(NULL); for cleanup, which should handle resource release upon thread exit without requiring pthread_detach.

Thanks again for catching this!

Best,
Vishwa

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.

4 participants