bpo-33015: Add a wrapper for thread function in PyThread_start_new_thread #6008
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
izbyshev
reviewed
Mar 10, 2018
Python/thread_pthread.h
Outdated
| typedef struct { | ||
| void (*func) (void *); | ||
| void *arg; | ||
| bool copied; |
There was a problem hiding this comment.
The flag must be volatile. Also, I'm not sure about the whole approach of busy-waiting in the parent thread until the child actually starts execution. Maybe malloc'ing the structure is preferable. Need the opinion of a core dev here.
There was a problem hiding this comment.
Thank you for your review.
Why does the flag need to be volatile? The release fence should be sufficient to ensure that the compiler doesn't do anything funny even if it does IPA. As for busy waiting vs having a mutex lock, I chose the former as a tradeoff since the synchronization requirement is fairly minor and didn't seem to justify the additional context switch. I don't have a strong opinion either way so if a mutex is desirable I'll change my patch accordingly.
malloc won't really solve anything since it will also require synchronization to free the block, unless you're proposing leaking that block, i.e. not freeing it at all.
I just noticed that the CI failed on the patch, I'll fix it up and re-post.
There was a problem hiding this comment.
Why does the flag need to be volatile?
You check the flag in the parent thread. Unless the compiler understands that the structure escapes into another thread, it may optimize the check way. I've not verified it, but the reason of the hang in the CI might be just that.
malloc won't really solve anything since it will also require synchronization to free the block
I meant malloc + ownership transfer to the child, so that the child must free the block when it's done. It's the same as in PyThread_start_new_thread in Python/thread_nt.h.
There was a problem hiding this comment.
Yeah you're right, the malloc way can actually be easier, I just didn't see it at first. Updated patch coming up.
Likewise for the volatile usage (to address all of the review comments); I was conflating two different issues and came to the wrong conclusion.
izbyshev
suggested changes
Mar 10, 2018
| @@ -0,0 +1,3 @@ | |||
| Add a wrapper for thread function in PyThread_start_new_thread | |||
|
|
|||
| PyThread_start_new_thread accepts a function of type `void (*) (void *)`, which does not match with the pthread_create function callback prototype `void () (void *)`. This results in an invalid function cast warning with gcc8. Fix this by wrapping the function in an internal pthread function callback that returns NULL. | |||
There was a problem hiding this comment.
I think that NEWS entries are usually short and don't explain the details (those are in the commit message), so I'd rephrase it (see other NEWS entries for examples).
Python/thread_pthread.h
Outdated
|
|
||
| func(arg); | ||
|
|
||
| free (fn); |
There was a problem hiding this comment.
Please use PyMem_RawMalloc/PyMem_RawFree like in other parts of the module (I've used malloc as a generic term before, sorry).
It's better to free it before calling func.
Please follow PEP 7 here and in other places (here, there should be no spaces between the function name and the parenthesis).
|
Hi, any update on this PR? |
izbyshev
approved these changes
Apr 22, 2018
|
Hello, any update on this? |
|
Hello, Ping! |
|
@zooba Would you look at this PR? |
|
Done, I also fixed up the dates for the change. |
|
See also PR #6748 and https://bugs.python.org/issue33012 |
vstinner
requested changes
Oct 23, 2018
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
pitrou
requested changes
Oct 23, 2018
|
Based on conversations with C / C++ experts at work, this approach seems correct (one even came up with an effectively identical proposal). Yes, it is unfortunate that we're in this situation due to the mismatched I agree with @pitrou 's code review comments. |
pitrou
approved these changes
Oct 25, 2018
|
This looks good to me. I will let other people comment just in case, otherwise merge soon. |
vstinner
requested changes
Oct 29, 2018
Python/thread_pthread.h
Outdated
|
|
||
| void (*func)(void *) = pfn->func; | ||
| void *arg = pfn->arg; | ||
|
|
There was a problem hiding this comment.
nitpick: please move this empty line after the free, not before :-) to make it clear that the memory is freed when the function is called.
Python/thread_pthread.h
Outdated
| } pythread_fn; | ||
|
|
||
| static void * | ||
| pythread_helper_fn(void *fn) |
There was a problem hiding this comment.
nitpick: i suggest to rename it to "pythread_wrapper". To me, it's more a wrapper than an helper ;-)
Python/thread_pthread.h
Outdated
| typedef struct { | ||
| void (*func) (void *); | ||
| void *arg; | ||
| } pythread_fn; |
There was a problem hiding this comment.
I suggest to rename it to pythread_callback since it's not only a function but function+arguments (or maybe pythread_func).
Python/thread_pthread.h
Outdated
| @@ -188,21 +213,32 @@ PyThread_start_new_thread(void (*func)(void *), void *arg) | |||
| pthread_attr_setscope(&attrs, PTHREAD_SCOPE_SYSTEM); | |||
| #endif | |||
|
|
|||
| pythread_fn *fn = (pythread_fn *)PyMem_RawMalloc(sizeof (pythread_fn)); | |||
Python/thread_pthread.h
Outdated
| if (status != 0) | ||
|
|
||
| if (status != 0) { | ||
| PyMem_RawFree((void *)fn); |
PyThread_start_new_thread accepts a function of type void (*) (void *), which does not match with the pthread_create function callback prototype void *(*) (void *). This is undefined behaviour and hence results in an invalid function cast warning with gcc8. Fix this by wrapping the function in an internal pthread function callback that returns NULL.
* Rename pythread_fn to pythread_callback * Rename pythread_helper_fn() to pythread_wrapper() * Rewrite the comment
* Rename "fn" to "callback"
* Add { ... } to if
* Remove useless cast
|
Since @siddhesh didn't reply since one month, I took the liberty of rebasing his change and apply my proposed cleanup changes. |
|
Sorry about that, I've been tied up and hence couldn't get to this. Thanks for cleaning it up for me. |
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Nov 30, 2018
Fix an undefined behaviour in the pthread implementation of PyThread_start_new_thread(): add a function wrapper to always return NULL. Add pythread_callback struct and pythread_wrapper() to thread_pthread.h. (cherry picked from commit 9eea6ea) Co-authored-by: Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com>
|
GH-10821 is a backport of this pull request to the 3.7 branch. |
|
Sorry, @siddhesh and @vstinner, I could not cleanly backport this to |
|
Sorry, @siddhesh and @vstinner, I could not cleanly backport this to |
|
GH-10822 is a backport of this pull request to the 3.6 branch. |
|
GH-10823 is a backport of this pull request to the 2.7 branch. |
miss-islington
added a commit
that referenced
this pull request
Nov 30, 2018
Fix an undefined behaviour in the pthread implementation of PyThread_start_new_thread(): add a function wrapper to always return NULL. Add pythread_callback struct and pythread_wrapper() to thread_pthread.h. (cherry picked from commit 9eea6ea) Co-authored-by: Siddhesh Poyarekar <siddhesh.poyarekar@gmail.com>
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PyThread_start_new_thread accepts a function of type
void (*) (void *), which does not match with the pthread_create function callback prototypevoid *(*) (void *). This results in an invalid function cast warning with gcc8. Fix this by wrapping the function in an internal pthread function callback that returns NULL.https://bugs.python.org/issue33015