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.

Sign up for GitHub

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

Jump to bottom

CFE-4401: Adjusted isreadable() to work without pthread_cancel when not available #5590

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

CFE-4401: Adjusted isreadable() to work without pthread_cancel when not available #5590

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cb70ddb
Adjusted use of pthreads for Android/Termux platform
craigcomstock Jul 8, 2024
bb794bf
Take patch in downstream termux-packages project and adjust to use HA…
craigcomstock Jul 26, 2024
c3504d3
updated termux make/install instructions
craigcomstock Jul 29, 2024
dd7fe67
help compilation from source on android termux platform
craigcomstock Jul 29, 2024
0078633
transition to MaskTerminationSignalsInThread() (from libntech soon) a…
craigcomstock Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,14 @@ CC="$PTHREAD_CC"
CFLAGS="$PTHREAD_CFLAGS $CFLAGS"
LIBS="$PTHREAD_LIBS $LIBS"

dnl ######################################################################
dnl libpromises/evalfunction.c isreadable() uses pthread_cancel so check
dnl if available and reverts to pthread_kill if not.
dnl This check must come after ACX_PTHREAD is called so that CC, CFLAGS,
dnl and LIBS are appropriately set for the local pthreads situation.
dnl ######################################################################
AC_CHECK_FUNCS([pthread_cancel])

dnl ######################################################################
dnl Whether to build extensions as builtin extensions or a separate
dnl plugin. The default is plugin.
Expand Down
23 changes: 23 additions & 0 deletions libpromises/evalfunction.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8513,10 +8513,27 @@ struct IsReadableThreadData
FnCallResult result;
};

#ifndef HAVE_PTHREAD_CANCEL
#define PTHREAD_CANCELED ((void *)-1)
static void ThreadSignalHandler(int signum)
{
pthread_exit(PTHREAD_CANCELED);
}
#endif

static void *IsReadableThreadRoutine(void *data)
{
assert(data != NULL);

#ifndef HAVE_PTHREAD_CANCEL
struct sigaction actions;
memset(&actions, 0, sizeof(actions));
sigemptyset(&actions.sa_mask);
actions.sa_flags = 0;
actions.sa_handler = ThreadSignalHandler;
sigaction(SIGUSR2, &actions, NULL);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

#endif

struct IsReadableThreadData *const thread_data = data;

// Give main thread time to call pthread_cond_timedwait(3)
Expand Down Expand Up @@ -8668,7 +8685,11 @@ static FnCallResult FnCallIsReadable(ARG_UNUSED EvalContext *const ctx,
"Read operation timed out, exceeded %ld seconds.", path,
timeout);

#ifdef HAVE_PTHREAD_CANCEL
ret = pthread_cancel(thread_data.thread);
#else
ret = pthread_kill(thread_data.thread, SIGUSR2);
Copy link
Contributor Author

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

#endif
if (ret != 0)
{
Log(LOG_LEVEL_ERR, "Failed to cancel thread");
Expand Down Expand Up @@ -8700,10 +8721,12 @@ static FnCallResult FnCallIsReadable(ARG_UNUSED EvalContext *const ctx,
return FnFailure();
}

#ifdef HAVE_PTHREAD_CANCEL
if (status == PTHREAD_CANCELED)
{
Log(LOG_LEVEL_DEBUG, "Thread was canceled");
}
#endif

return result;
}
Expand Down
Loading