Implement non-standard pthread_tryjoin and pthread_timedjoin. #72654

ClaCodes · 2024-05-13T06:43:21Z

Implement the non-standard facilities for joining pthreads.

Open Questions

naming _np for non-standard as gnu does?
test allowed to sleep 300 ms?
use maximum value of time_t to represent forever in the timespec, which is the correct/portable makro?
convert timespec to timeout -> handle overflow?

This is my first PR, so let me know, if something formal is wrong. @cfriedt please let me know your opinion on this.

github-actions · 2024-05-13T06:44:02Z

Hello @ClaCodes, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

cfriedt · 2024-05-13T10:44:35Z

@ClaCodes - I would definitely add the _np suffix (and also tests passing).

I'll add a more detailed review in a few minutes.

cfriedt

I think we should convert to k_timeout at the point of entry, rename pthread_join() to static pthread_timedjoin_internal(), also accept a k_timeout_t timeout, and then the other join functions can be 1-line wrappers for the internal version.

Additionally, please add tests in a second commit and follow the commit guidelines.

Tests should be directed to individual functions, so add

ZTEST(pthread, test_pthread_timedjoin)
ZTEST(pthread, test_pthread_tryjoin)

following the existing conventions (see git log), where each test case tests various permutations of invalid input and then valid input.

cfriedt · 2024-05-13T11:21:27Z

include/zephyr/posix/pthread.h

Suggested change

int pthread_timedjoin_np(pthread_t thread, void **status, const struct timespec abstime);

int pthread_timedjoin_np(pthread_t thread, void **status, const struct timespec *abstime);

cfriedt · 2024-05-13T11:38:53Z

lib/posix/options/pthread.c

One possibly cleaner way to go about this is to convert all absolute struct timespec to k_timeout at the point of entry, and have an internal pthread_timedjoin_internal(pthread_t th, void **status, k_timeout_t timeout) (where the timeout is already specified as a k_timeout_t instead of struct timespec).

That way, e.g.

pthread_join(th, st) -> pthread_timedjoin_internal(th, st, K_FOREVER)

pthread_tryjoin(th, st) -> pthread_timedjoin_internal(th, st, K_NO_WAIT)

pthread_timedjoin_np(th, st, to) -> pthread_timedjoin_internal(th, st, K_MSEC(timespec_to_timeoutms(to)))

cfriedt · 2024-05-13T11:50:42Z

@ClaCodes - be sure to tests prior to pushing via

twister -i -T tests/posix

and check compliance via

./scripts/checkpatch.pl -g <commit-before-yours>..

ClaCodes · 2024-06-01T16:28:07Z

@cfriedt I tried to address your suggestions and used checkpatch. I am unable to run the test locally because I am on an unsupported system. Will have to rely on the CI for that currently.

cfriedt

I think we need to check the return value of k_thread_join() now that the assertion is gone.

Otherwise, looks ok.

I'm just on mobile atm, so cannot look in detail.

Please double check that all meaningful input permutations are made in the testsuite as well.

lib/posix/options/pthread.c

ClaCodes · 2024-06-01T19:31:55Z

Please double check that all meaningful input permutations are made in the testsuite as well.

There is probably more that could be done. For now idea was:

tryjoin:
- spawn thread that ends after 1s
- immediately tryjoin expect EBUSY
- sleep 1s
- tryjoin expect success
timedjoin:
- create thread that ends after 1s
- Test -EINVALID for negativ seconds, negative nanoseconds and nanonseconds == NSEC_PER_SEC
- timed join with timeout less then 1s after start, excpect -ETIMEDOUT
- timed join with timeout later then 1s after start, excpect success

cfriedt

A null check would be good before dereferencing abstime

lib/posix/options/pthread.c

github-actions · 2024-08-05T00:30:23Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

cfriedt

Probably could use a rebase, and there are a couple of platforms where tests are not passing.

tests/posix/common/src/pthread.c

lib/posix/options/pthread.c

cfriedt · 2024-08-05T11:19:27Z

@ClaCodes - would be nice to get this in - can you make some changes and resubmit?

ClaCodes · 2024-08-06T20:33:12Z

@ClaCodes - would be nice to get this in - can you make some changes and resubmit?

I rebased and tried to make test more robust by adding a bit more time. I am not sure, why some test are failing. I might have some time to look into it more, if adding more time was not good enough.
Maybe you could have a look at the test to see if I have some wrong assumptions.

cfriedt · 2024-11-29T23:23:05Z

@ClaCodes - this is pretty close - just needs a nudge past the finish line 🏁

ClaCodes · 2024-12-01T16:04:31Z

Unfortunately, I am still having trouble with the tests. If someone wants to pick this up, you are welcome to. Otherwise I will be closing.

Thanks for understanding and sorry.

tests/posix/common/src/pthread.c

cfriedt · 2024-12-03T03:04:23Z

@ClaCodes - there is a subtle bug that needs to be fixed in the commit titled "posix: pthread: implement non-standard try-join and timed-join".

diff --git a/lib/posix/options/pthread.c b/lib/posix/options/pthread.c
index 5dfd6599784..db5d37fc8cc 100644
--- a/lib/posix/options/pthread.c
+++ b/lib/posix/options/pthread.c
@@ -1112,6 +1112,11 @@ static int pthread_timedjoin_internal(pthread_t pthread, void **status, k_timeou
        }
 
        ret = k_thread_join(&t->thread, timeout);
+       if (ret != 0) {
+               SYS_SEM_LOCK(&pthread_pool_lock) {
+                       t->attr.detachstate = PTHREAD_CREATE_JOINABLE;
+               }
+       }
        if (ret == -EBUSY) {
                return EBUSY;
        } else if (ret == -EAGAIN) {

ClaCodes · 2024-12-08T15:16:16Z

@ClaCodes - there is a subtle bug that needs to be fixed in the commit titled "posix: pthread: implement non-standard try-join and timed-join".

diff --git a/lib/posix/options/pthread.c b/lib/posix/options/pthread.c
index 5dfd6599784..db5d37fc8cc 100644
--- a/lib/posix/options/pthread.c
+++ b/lib/posix/options/pthread.c
@@ -1112,6 +1112,11 @@ static int pthread_timedjoin_internal(pthread_t pthread, void **status, k_timeou
        }
 
        ret = k_thread_join(&t->thread, timeout);
+       if (ret != 0) {
+               SYS_SEM_LOCK(&pthread_pool_lock) {
+                       t->attr.detachstate = PTHREAD_CREATE_JOINABLE;
+               }
+       }
        if (ret == -EBUSY) {
                return EBUSY;
        } else if (ret == -EAGAIN) {

@cfriedt Thank you for sending this patch.
I have applied it and rebased.
Let me know, what you think

ghost · 2024-12-09T14:54:18Z

@ClaCodes - there is a subtle bug that needs to be fixed in the commit titled "posix: pthread: implement non-standard try-join and timed-join".

diff --git a/lib/posix/options/pthread.c b/lib/posix/options/pthread.c
index 5dfd6599784..db5d37fc8cc 100644
--- a/lib/posix/options/pthread.c
+++ b/lib/posix/options/pthread.c
@@ -1112,6 +1112,11 @@ static int pthread_timedjoin_internal(pthread_t pthread, void **status, k_timeou
        }
 
        ret = k_thread_join(&t->thread, timeout);
+       if (ret != 0) {
+               SYS_SEM_LOCK(&pthread_pool_lock) {
+                       t->attr.detachstate = PTHREAD_CREATE_JOINABLE;
+               }
+       }
        if (ret == -EBUSY) {
                return EBUSY;
        } else if (ret == -EAGAIN) {

@cfriedt Thank you for sending this patch. I have applied it and rebased. Let me know, what you think

@cfriedt This seems to have fixed the tests. Could you resolve all conversations you consider resolved and then I will attempt to fix the rests (naming, return codes, ... etc.).

lib/posix/options/pthread.c

ycsin · 2024-12-09T17:25:45Z

tests/posix/common/src/pthread.c

The comments should match the updated sleep time

adjusted. Nice catch.

ycsin · 2024-12-09T17:28:28Z

tests/posix/common/src/pthread.c

I think it would be nice to pass the sleep time as arg as well:

Suggested change

zassert_ok(pthread_create(&th, NULL, timedjoin_thread, NULL));

useconds_t sleep_time;

...

/* Creating a threads that exits after 200ms */

sleep_time = USEC_PER_MSEC * 200U;

zassert_ok(pthread_create(&th, NULL, timedjoin_thread, (void *)sleep_time);

but probably just personal preference, so non-blocking

Is a useconds_t guaranteed to fit into a void *?
For now, I have adjusted the comment.

It's not a bad suggestion, but INT_TO_POINTER() and POINTER_TO_INT() should be used.

Also, we should use "create a thread" instead of "create a threads".

I have added this. But with type int instead of useconds_t, so that I could use the INT_TO_POINTER/POINTER_TO_INT macros. Let me know what you think.

ycsin · 2024-12-09T17:29:54Z

tests/posix/common/src/pthread.c

Could have use the sleep_time variable if my suggestion above is adopted

I generally like the sugestion. See my concern above.

ycsin · 2024-12-09T17:32:05Z

tests/posix/common/src/pthread.c

Suggested change

/* Attempting to join immediately should fail */

/* Attempting to join without blocking should fail */

I was wondering why the comment says 'join immediately' when we slept for 100ms above

Fair enough. I adjusted to /* Attempting to join, when thread is still running, should fail */, what do you think?

ycsin · 2024-12-09T17:33:00Z

tests/posix/common/src/pthread.c

Just thought that this might be clearer (at least for me)

Suggested change

/* Attempting to join immediately should succeed now */

/* Attempting to join without blocking should succeed now */

applied. Thanks.

cfriedt · 2024-12-10T01:14:46Z

tests/posix/common/src/pthread.c

Suggested change

/* Creating a threads that exits after 200ms*/

/* Creating a thread that exits after 200ms*/

cfriedt · 2024-12-10T01:15:37Z

tests/posix/common/src/pthread.c

Also, we should use "create a thread" instead of "create a threads".

ClaCodes · 2024-12-10T07:36:51Z

@cfriedt @ycsin Thanks for the suggestions and fixes.
I tried to apply them. Please have another look.

tests/posix/common/src/pthread.c

These functions can be used to join pthreads in a non-standard way. The function pthread_tryjoin will not block and simply test whether the thread has exited already. The function pthread_timed_join will only block until the specified time. The functions are wrappers for calling the k_thread_join with timeout K_NO_WAIT and with a specific timeout as opposed to calling it with K_FOREVER. Signed-off-by: Cla Galliard <[email protected]>

Test for the non-standard but useful pthread_timed_join- and pthread_try_join-functions. Signed-off-by: Cla Galliard <[email protected]>

ClaCodes · 2024-12-12T06:40:36Z

@cfriedt @ycsin It looks like it is passing. Is there anything missing?

ClaCodes · 2024-12-15T11:06:52Z

@ycsin @cfriedt Thank you for your support on this.
It still says 'Merging is blocked`. Do you require any more action from my side?

ycsin · 2024-12-15T11:21:12Z

@ycsin @cfriedt Thank you for your support on this.
It still says 'Merging is blocked`. Do you require any more action from my side?

Just sit back and relax - you will be notified if there's another review/ when it get merged

github-actions · 2024-12-15T18:26:18Z

Hi @ClaCodes!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge.

Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁

zephyrbot added the area: POSIX POSIX API Library label May 13, 2024

zephyrbot requested review from cfriedt and ycsin May 13, 2024 06:43

zephyrbot assigned cfriedt May 13, 2024

cfriedt requested changes May 13, 2024

View reviewed changes

ClaCodes force-pushed the pthread_timed_join branch from 58beb60 to ab80996 Compare June 1, 2024 16:26

ClaCodes force-pushed the pthread_timed_join branch 2 times, most recently from 451fa74 to 2dddd68 Compare June 1, 2024 17:14

ClaCodes changed the title ~~RFC: Implement non-standart pthread_tryjoin and pthread_timedjoin.~~ Implement non-standart pthread_tryjoin and pthread_timedjoin. Jun 1, 2024

cfriedt requested changes Jun 1, 2024

View reviewed changes

lib/posix/options/pthread.c Outdated Show resolved Hide resolved

ClaCodes force-pushed the pthread_timed_join branch from 2dddd68 to db20d74 Compare June 1, 2024 19:30

ClaCodes force-pushed the pthread_timed_join branch 2 times, most recently from 5063dc3 to 2e47c43 Compare June 1, 2024 21:09

cfriedt reviewed Jun 1, 2024

View reviewed changes

lib/posix/options/pthread.c Outdated Show resolved Hide resolved

ClaCodes force-pushed the pthread_timed_join branch 3 times, most recently from 99e26d2 to 3ba76d2 Compare June 5, 2024 05:53

github-actions bot added the Stale label Aug 5, 2024

cfriedt removed the Stale label Aug 5, 2024

cfriedt reviewed Aug 5, 2024

View reviewed changes

tests/posix/common/src/pthread.c Outdated Show resolved Hide resolved

tests/posix/common/src/pthread.c Outdated Show resolved Hide resolved

lib/posix/options/pthread.c Outdated Show resolved Hide resolved

ClaCodes force-pushed the pthread_timed_join branch 2 times, most recently from 0532093 to 69417b0 Compare August 6, 2024 20:31

ClaCodes force-pushed the pthread_timed_join branch from eb7e5b2 to c7382b2 Compare December 1, 2024 08:09

cfriedt changed the title ~~Implement non-standart pthread_tryjoin and pthread_timedjoin.~~ Implement non-standard pthread_tryjoin and pthread_timedjoin. Dec 1, 2024

cfriedt reviewed Dec 1, 2024

View reviewed changes

tests/posix/common/src/pthread.c Outdated Show resolved Hide resolved

ClaCodes force-pushed the pthread_timed_join branch from c7382b2 to b9fa2eb Compare December 1, 2024 20:51

ClaCodes force-pushed the pthread_timed_join branch from b9fa2eb to 9addfd3 Compare December 8, 2024 15:13

cfriedt previously approved these changes Dec 9, 2024

View reviewed changes

ycsin reviewed Dec 9, 2024

View reviewed changes

ClaCodes dismissed cfriedt’s stale review via d289583 December 9, 2024 20:33

ClaCodes force-pushed the pthread_timed_join branch from 9addfd3 to d289583 Compare December 9, 2024 20:33

cfriedt reviewed Dec 10, 2024

View reviewed changes

ClaCodes force-pushed the pthread_timed_join branch from d289583 to 2d65e23 Compare December 10, 2024 07:34

cfriedt reviewed Dec 10, 2024

View reviewed changes

tests/posix/common/src/pthread.c Outdated Show resolved Hide resolved

ClaCodes added 2 commits December 10, 2024 17:40

tests/posix: Add test for pthread_timed_join and pthread_try_join

3bbce6e

Test for the non-standard but useful pthread_timed_join- and pthread_try_join-functions. Signed-off-by: Cla Galliard <[email protected]>

ClaCodes force-pushed the pthread_timed_join branch from 2d65e23 to 3bbce6e Compare December 10, 2024 16:40

cfriedt approved these changes Dec 12, 2024

View reviewed changes

cfriedt requested a review from ycsin December 14, 2024 22:43

ycsin approved these changes Dec 14, 2024

View reviewed changes

kartben merged commit b0baeee into zephyrproject-rtos:main Dec 15, 2024
26 checks passed

ClaCodes deleted the pthread_timed_join branch October 19, 2025 19:31

	int pthread_timedjoin_np(pthread_t thread, void **status, const struct timespec abstime);
	int pthread_timedjoin_np(pthread_t thread, void *status, const struct timespec abstime);

-	zassert_ok(pthread_create(&th, NULL, timedjoin_thread, NULL));
+	useconds_t sleep_time;
+	...
+	/* Creating a threads that exits after 200ms */
+	sleep_time = USEC_PER_MSEC * 200U;
+	zassert_ok(pthread_create(&th, NULL, timedjoin_thread, (void *)sleep_time);

	/* Attempting to join immediately should fail */
	/* Attempting to join without blocking should fail */

	/* Creating a threads that exits after 200ms*/
	/* Creating a thread that exits after 200ms*/

Implement non-standard pthread_tryjoin and pthread_timedjoin. #72654

Implement non-standard pthread_tryjoin and pthread_timedjoin. #72654

Uh oh!

Conversation

ClaCodes commented May 13, 2024 • edited by cfriedt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Open Questions

Uh oh!

github-actions bot commented May 13, 2024

Uh oh!

cfriedt commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfriedt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfriedt commented May 13, 2024

Uh oh!

ClaCodes commented Jun 1, 2024

Uh oh!

cfriedt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ClaCodes commented Jun 1, 2024

Uh oh!

cfriedt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 5, 2024

Uh oh!

cfriedt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfriedt commented Aug 5, 2024

Uh oh!

ClaCodes commented Aug 6, 2024

Uh oh!

cfriedt commented Nov 29, 2024

Uh oh!

ClaCodes commented Dec 1, 2024

Uh oh!

Uh oh!

cfriedt commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ClaCodes commented Dec 8, 2024

Uh oh!

ghost commented Dec 9, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ClaCodes commented May 13, 2024 •

edited by cfriedt

Loading

cfriedt commented May 13, 2024 •

edited

Loading

cfriedt left a comment •

edited

Loading

cfriedt commented Dec 3, 2024 •

edited

Loading