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

implement pthread_cancel -- DO NOT MERGE, ARCHIVE ONLY. #3

Closed
wants to merge 8 commits into from

Conversation

dachalco
Copy link

I don't expect the test functions to make it through, but to be used for reviewer verification.
This is per aws/amazon-freertos#587

Copy link
Contributor

@yuhui-zheng yuhui-zheng left a comment

Choose a reason for hiding this comment

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

Please rebase and in your PR include changes to pthread_cancel() related only.

I did not fully review pthread_cancel(), let's get the rest out of the way first.

@yuhui-zheng
Copy link
Contributor

I'm aware we haven't have everything fully setup in this repo. Please include some detail about what level of test you have done (e.g. compiled || run on which platform || etc.). (Funny I'm commenting on test, given we can't accept test cases yet in this repo.)

…RTOS-POSIX

# Conflicts:
#	FreeRTOS-Plus/Demo/FreeRTOS_Plus_POSIX_with_actor_Windows_Simulator/FreeRTOSConfig.h
#	FreeRTOS-Plus/Demo/FreeRTOS_Plus_POSIX_with_actor_Windows_Simulator/posix_demo.c
#	FreeRTOS-Plus/Demo/FreeRTOS_Plus_POSIX_with_actor_Windows_Simulator/posix_demo.h
@dachalco dachalco changed the title handle pthread_join(null) + implement pthread_cancel w/ tests implement pthread_cancel w/ tests Feb 22, 2020
@yuhui-zheng yuhui-zheng changed the title implement pthread_cancel w/ tests implement pthread_cancel Feb 25, 2020

/* Threads may already be successfully ended elsewhere and pending full deletion in idle task. */
/* Note the the xJoinBarrier is a binary sem, so no new queue items are made once its already given */
if( pxThread && pxThread->xTaskHandle && ( eDeleted != eTaskGetState( pxThread->xTaskHandle ) ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

I finished my meditation, and the implementation proposed here I believe is incorrect. I'm leaving this PR open mainly for documentation purpose. I'm commenting my reasoning below in detail.

The implementation here suspends/deletes the task without checking for "cancelability state". The detailed description of "thread cancellation" and "cancellation points" are described in POSIX Threads chapter https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_05.

Specifically:

Cancellation is controlled by the cancellation control functions. Each thread maintains its own cancelability state. Cancellation may only occur at cancellation points or when the thread is asynchronously cancelable.

(A direct counter example to the implementation in this PR is -- what if the thread being cancelled is holding a mutex? If it's detached, we can't just delete it... )

When talking about FreeRTOS+POSIX, mainly as an adapter layer to FreeRTOS APIs, we need to investigate a couple of things:

  1. How to track per thread cencelability state. (See TCB definition, and consider how to add the state in either POSIX layer or kernel layer.)
  2. Thread state machine and transit condition, see what thread cancellation means in each state. (And see how cancellation point apply in such context.)
  3. If we are talking about kernel changes, please have performance reasonings ready so that we could pick an implementation.

The quick summary is thread cancelation cannot be done with a single function as proposed in this PR.

@yuhui-zheng yuhui-zheng changed the title implement pthread_cancel implement pthread_cancel -- DO NOT MERGE, ARCHIVE ONLY. Feb 25, 2020
@yuhui-zheng yuhui-zheng mentioned this pull request Feb 26, 2020
@dachalco dachalco closed this Jul 24, 2020
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