-
Notifications
You must be signed in to change notification settings - Fork 185
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
CFE-4401: Adjusted isreadable() to work without pthread_cancel when not available #5590
base: master
Are you sure you want to change the base?
Conversation
craigcomstock
commented
Jul 26, 2024
- Adjusted use of pthreads for Android/Termux platform
- Take patch in downstream termux-packages project and adjust to use HAVE_PTHREAD_CANCEL instead of ANDROID
Android/Termux doesn't have pthread_cancel so use pthred_kill instead Ticket: CFE-4401 Changelog: title (cherry picked from commit ebd52a29238122c4bcc2f86d2313f9f4a258243c) (cherry picked from commit 1e965d3)
…VE_PTHREAD_CANCEL instead of __ANDROID__ https://github.com/termux/termux-packages/blob/master/packages/cfengine/pthread_cancel.patch Ticket: CFE-4401 Changelog: none (cherry picked from commit 5cebd75)
60356f4
to
bb794bf
Compare
ret = pthread_cancel(thread_data.thread); | ||
#else | ||
ret = pthread_kill(thread_data.thread, SIGUSR2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larsewi you had mentioned in my previous PR comment
Is it safe to join a "killed" thread? Will this call the signal handler installed by the agent. If so, is this fine? Also the man pthread_kill(3) says:
And so here I am using the downstream patch which sends a kill signal and adds a signal handler to which then calls pthread_exit
which is the "other option" of how the joined thread could be terminated at the appropriate timeout moment.
After a canceled thread has terminated, a join with that thread
using pthread_join(3) obtains PTHREAD_CANCELED as the thread's
exit status. (Joining with a thread is the only way to know that
cancelation has completed.)
from https://www.man7.org/linux/man-pages/man3/pthread_cancel.3.html
@xtkoba, I took your patch at https://github.com/termux/termux-packages/blob/master/packages/cfengine/pthread_cancel.patch as inspiration to improve my earlier attempt. I am also working on a "cfengine: Bump 3.24.0" PR. Getting this patch upstream will have to wait for 3.24.1 release if we merge the PR here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cherry-picked lines mention commits that are not in this repository. I don't think we need them. And let's squash the commits, please. The first one is not good enough alone.
libpromises/evalfunction.c
Outdated
sigemptyset(&actions.sa_mask); | ||
actions.sa_flags = 0; | ||
actions.sa_handler = ThreadSignalHandler; | ||
sigaction(SIGUSR2, &actions, NULL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer SIGHUP
or some other signal that semantically matches the case? In fact, I think we should block many common signals in the new thread, we don't want handlers for SIGINT
etc. to be handled in it. So I'd extend this to something like what we do in cf-reactor and used one of the other signals here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I agree with SIGHUP/etc.
The reason for splitting commits was to acknowledge and make clear the use of another persons contribution.
I do agree that it makes more sense to have commits which are sufficient and complete on their own but also recognize that sometimes we make "mistakes" in a commit and then have to come back and fix them, sort of like in this case here.
Actually the "cherry-picked" comments are not needed, these are simply commits and not cherry-picked from someplace permanent or interesting (just another branch or my forked repo so an artifact of git operations). I will remove the cherry-pick comments and consider squashing after I fixup the signal handling. |
This is a work-in-progress now as I am refactoring a few things based on code review. No need to comment until I un-draft the PR. |
Thanks for the notice and double thanks for the refactoring! |