Skip to content

Commit

Permalink
Fix #642, make OS_TaskDelete synchronous
Browse files Browse the repository at this point in the history
In the POSIX implementation, OS_TaskDelete was implemented in a
deferred manner - the API call was a request, and the actual
deletion occured sometime thereafter.  This is a problem if the
task is running code within a dynamically loaded module, and the
intent is to delete the task so the module can be unloaded.  In
this case the app needs to be certain that the task has actually
been deleted before unloading can be done safely.

To do this requires use of pthread_join() on POSIX which confirms
that the task has exited.  However, this is a (potentially) blocking
call, so to do this requires rework of the EXCLUSIVE lock mode
such that the OSAL lock is _not_ held during the join operation.
  • Loading branch information
jphickey committed Dec 9, 2020
1 parent 5c61560 commit 2d876a5
Show file tree
Hide file tree
Showing 10 changed files with 239 additions and 85 deletions.
86 changes: 79 additions & 7 deletions src/os/posix/src/os-impl-tasks.c
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,16 @@ int32 OS_Posix_InternalTaskCreate_Impl(pthread_t *pthr, osal_priority_t priority
return (OS_ERROR);
}

/*
** Set the thread to be joinable by default
*/
return_code = pthread_attr_setdetachstate(&custom_attr, PTHREAD_CREATE_JOINABLE);
if (return_code != 0)
{
OS_DEBUG("pthread_attr_setdetachstate error in OS_TaskCreate: %s\n", strerror(return_code));
return (OS_ERROR);
}

/*
** Test to see if the original main task scheduling priority worked.
** If so, then also set the attributes for this task. Otherwise attributes
Expand Down Expand Up @@ -541,12 +551,6 @@ int32 OS_Posix_InternalTaskCreate_Impl(pthread_t *pthr, osal_priority_t priority
** Do not treat anything bad that happens after this point as fatal.
** The task is running, after all - better to leave well enough alone.
*/
return_code = pthread_detach(*pthr);
if (return_code != 0)
{
OS_DEBUG("pthread_detach error in OS_TaskCreate: %s\n", strerror(return_code));
}

return_code = pthread_attr_destroy(&custom_attr);
if (return_code != 0)
{
Expand Down Expand Up @@ -583,6 +587,33 @@ int32 OS_TaskCreate_Impl(const OS_object_token_t *token, uint32 flags)
return return_code;
} /* end OS_TaskCreate_Impl */

/*----------------------------------------------------------------
*
* Function: OS_TaskDetach_Impl
*
* Purpose: Implemented per internal OSAL API
* See prototype for argument/return detail
*
*-----------------------------------------------------------------*/
int32 OS_TaskDetach_Impl(const OS_object_token_t *token)
{
OS_impl_task_internal_record_t *impl;
int ret;

impl = OS_OBJECT_TABLE_GET(OS_impl_task_table, *token);

ret = pthread_detach(impl->id);

if (ret != 0)
{
OS_DEBUG("pthread_detach: Failed on Task ID = %lu, err = %s\n",
OS_ObjectIdToInteger(OS_ObjectIdFromToken(token)), strerror(ret));
return OS_ERROR;
}

return OS_SUCCESS;
}

/*----------------------------------------------------------------
*
* Function: OS_TaskMatch_Impl
Expand Down Expand Up @@ -616,6 +647,8 @@ int32 OS_TaskMatch_Impl(const OS_object_token_t *token)
int32 OS_TaskDelete_Impl(const OS_object_token_t *token)
{
OS_impl_task_internal_record_t *impl;
void *retval;
int ret;

impl = OS_OBJECT_TABLE_GET(OS_impl_task_table, *token);

Expand All @@ -625,7 +658,35 @@ int32 OS_TaskDelete_Impl(const OS_object_token_t *token)
** to cancel here is that the thread ID is invalid because it already exited itself,
** and if that is true there is nothing wrong - everything is OK to continue normally.
*/
pthread_cancel(impl->id);
ret = pthread_cancel(impl->id);
if (ret != 0)
{
OS_DEBUG("pthread_cancel: Failed on Task ID = %lu, err = %s\n",
OS_ObjectIdToInteger(OS_ObjectIdFromToken(token)), strerror(ret));

/* fall through (will still return OS_SUCCESS) */
}
else
{
/*
* Note that "pthread_cancel" is a request - and successful return above
* only means that the cancellation request is pending.
*
* pthread_join() will wait until the thread has actually exited.
*
* This is important for CFE, as task deletion often occurs in
* conjunction with an application reload - which means the next
* call is likely to be OS_ModuleUnload(). So is critical that all
* tasks potentially executing code within that module have actually
* been stopped - not just pending cancellation.
*/
ret = pthread_join(impl->id, &retval);
if (ret != 0)
{
OS_DEBUG("pthread_join: Failed on Task ID = %lu, err = %s\n",
OS_ObjectIdToInteger(OS_ObjectIdFromToken(token)), strerror(ret));
}
}
return OS_SUCCESS;

} /* end OS_TaskDelete_Impl */
Expand Down Expand Up @@ -731,6 +792,17 @@ int32 OS_TaskRegister_Impl(osal_id_t global_task_id)
{
int32 return_code;
OS_U32ValueWrapper_t arg;
int old_state;
int old_type;

/*
* Set cancel state=ENABLED, type=DEFERRED
* This should be the default for new threads, but
* setting explicitly to be sure that a pthread_join()
* will work as expected in case this thread is deleted.
*/
pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, &old_state);
pthread_setcanceltype(PTHREAD_CANCEL_DEFERRED, &old_type);

arg.opaque_arg = 0;
arg.id = global_task_id;
Expand Down
2 changes: 1 addition & 1 deletion src/os/posix/src/os-impl-timebase.c
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ int32 OS_TimeBaseCreate_Impl(const OS_object_token_t *token)
*/
for (idx = 0; idx < OS_MAX_TIMEBASES; ++idx)
{
if (OS_ObjectIdDefined(OS_global_timebase_table[idx].active_id) &&
if (OS_ObjectIdIsValid(OS_global_timebase_table[idx].active_id) &&
OS_impl_timebase_table[idx].assigned_signal != 0)
{
sigaddset(&local->sigset, OS_impl_timebase_table[idx].assigned_signal);
Expand Down
23 changes: 20 additions & 3 deletions src/os/shared/inc/os-shared-idmap.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,6 @@

#include <os-shared-globaldefs.h>

#define OS_OBJECT_EXCL_REQ_FLAG 0x0001

#define OS_OBJECT_ID_RESERVED ((osal_id_t) {0xFFFFFFFF})

/*
Expand All @@ -43,7 +41,6 @@ struct OS_common_record
osal_id_t active_id;
osal_id_t creator;
uint16 refcount;
uint16 flags;
};

/*
Expand Down Expand Up @@ -214,6 +211,26 @@ static inline void OS_ObjectIdCompose_Impl(osal_objtype_t idtype, uint32 idseria
*result = OS_ObjectIdFromInteger((idtype << OS_OBJECT_TYPE_SHIFT) | idserial);
}

/*-------------------------------------------------------------------------------------*/
/**
* @brief Check if an object ID represents a valid/active value.
*
* This tests that the ID value is within the range specifically used by
* valid OSAL IDs. This is smaller than the set of defined IDs.
*
* For example, the value of OS_OBJECT_ID_RESERVED is defined but not valid.
* So while OS_ObjectIdDefined() will match entries being actively created or
* deleted, OS_ObjectIdIsValid() will not.
*
* @param[in] object_id The object ID
* @returns true if table entry is valid
*/
static inline bool OS_ObjectIdIsValid(osal_id_t object_id)
{
osal_objtype_t objtype = OS_ObjectIdToType_Impl(object_id);
return (objtype > OS_OBJECT_TYPE_UNDEFINED && objtype < OS_OBJECT_TYPE_USER);
}

/*----------------------------------------------------------------
Function: OS_GetMaxForObjectType
Expand Down
10 changes: 10 additions & 0 deletions src/os/shared/inc/os-shared-task.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,16 @@ int32 OS_TaskMatch_Impl(const OS_object_token_t *token);
------------------------------------------------------------------*/
int32 OS_TaskCreate_Impl(const OS_object_token_t *token, uint32 flags);

/*----------------------------------------------------------------
Function: OS_TaskDetach_Impl
Purpose: Sets the thread so that the OS resources associated with the task
will be released when the thread exits itself
Returns: OS_SUCCESS on success, or relevant error code
------------------------------------------------------------------*/
int32 OS_TaskDetach_Impl(const OS_object_token_t *token);

/*----------------------------------------------------------------
Function: OS_TaskDelete_Impl
Expand Down
Loading

0 comments on commit 2d876a5

Please sign in to comment.