You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/06/10 17:19:34 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #1225: task_delete should remove all threads in the task
xiaoxiang781216 opened a new pull request #1225:
URL: https://github.com/apache/incubator-nuttx/pull/1225
## Summary
The current implementation just remove the main thread which isn't enough as the function name(task_delete) indicate.
## Impact
## Testing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1225: task_delete should remove all threads in the task
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1225:
URL: https://github.com/apache/incubator-nuttx/pull/1225#discussion_r438295600
##########
File path: sched/task/task_delete.c
##########
@@ -132,6 +133,26 @@ int task_delete(pid_t pid)
goto errout;
}
+ /* Check if the task to delete is the calling task */
+
+ if (pid == rtcb->pid)
+ {
+ /* If it is, then what we really wanted to do was exit. Note that we
+ * don't bother to unlock the TCB since it will be going away.
+ */
+
+ exit(EXIT_SUCCESS);
+ }
+
+#ifdef HAVE_GROUP_MEMBERS
+ /* Kill all of the children of the task. This will not kill the currently
+ * running task/pthread (this_task). It will kill the main thread of the
+ * task group if this_task is a pthread.
+ */
+
+ group_kill_children(dtcb);
Review comment:
Hmm... but group_kill_children() calls task_delete():
84 /* 'pid' could refer to the main task of the thread. That pid
85 * will appear in the group member list as well!
86 */
87
88 if ((rtcb->flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD)
89 {
90 ret = pthread_cancel(pid);
91 }
92 else
93 {
94 ret = task_delete(pid);
95 }
Is that okay?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo merged pull request #1225: sched: Make task_delete(getpid()) equal exit(EXIT_SUCCESS)
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #1225:
URL: https://github.com/apache/incubator-nuttx/pull/1225
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1225: task_delete should remove all threads in the task
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1225:
URL: https://github.com/apache/incubator-nuttx/pull/1225#discussion_r438303753
##########
File path: sched/task/task_delete.c
##########
@@ -132,6 +133,26 @@ int task_delete(pid_t pid)
goto errout;
}
+ /* Check if the task to delete is the calling task */
+
+ if (pid == rtcb->pid)
+ {
+ /* If it is, then what we really wanted to do was exit. Note that we
+ * don't bother to unlock the TCB since it will be going away.
+ */
+
+ exit(EXIT_SUCCESS);
+ }
+
+#ifdef HAVE_GROUP_MEMBERS
+ /* Kill all of the children of the task. This will not kill the currently
+ * running task/pthread (this_task). It will kill the main thread of the
+ * task group if this_task is a pthread.
+ */
+
+ group_kill_children(dtcb);
Review comment:
there is another patch which change the code to:
```
ret = pthread_cancel(pid);
```
Of course, the change only work after we unify the main and pthread difference(e.g. the cleanup stack and robust mutex). I will drop this patch now and resend after the unification patch pass the review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1225: task_delete should remove all threads in the task
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1225:
URL: https://github.com/apache/incubator-nuttx/pull/1225#discussion_r438295600
##########
File path: sched/task/task_delete.c
##########
@@ -132,6 +133,26 @@ int task_delete(pid_t pid)
goto errout;
}
+ /* Check if the task to delete is the calling task */
+
+ if (pid == rtcb->pid)
+ {
+ /* If it is, then what we really wanted to do was exit. Note that we
+ * don't bother to unlock the TCB since it will be going away.
+ */
+
+ exit(EXIT_SUCCESS);
+ }
+
+#ifdef HAVE_GROUP_MEMBERS
+ /* Kill all of the children of the task. This will not kill the currently
+ * running task/pthread (this_task). It will kill the main thread of the
+ * task group if this_task is a pthread.
+ */
+
+ group_kill_children(dtcb);
Review comment:
Hmm... but group_kill_children() calls task_delete():
84 /* 'pid' could refer to the main task of the thread. That pid
85 * will appear in the group member list as well!
86 */
87
88 if ((rtcb->flags & TCB_FLAG_TTYPE_MASK) == TCB_FLAG_TTYPE_PTHREAD)
89 {
90 ret = pthread_cancel(pid);
91 }
92 else
93 {
94 ret = task_delete(pid);
95 }
Is that okay? It doesn't look right. It looks to me like task_delete() will bet called recursively, perhaps even leading to a lock-up infinite loop?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org