bpo-16500: Allow registering at-fork handlers#1715
Conversation
|
@pitrou, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @rhettinger and @tim-one to be potential reviewers. |
| PyAPI_FUNC(void) PyOS_InitInterrupts(void); | ||
| PyAPI_FUNC(void) PyOS_AfterFork(void); | ||
| #ifdef HAVE_FORK | ||
| PyAPI_FUNC(void) PyOS_BeforeFork(void); |
There was a problem hiding this comment.
Question: should these functions be defined even when fork() isn't available?
There was a problem hiding this comment.
They can be used only by the code that calls fork() (or similar functions).
|
|
||
| #ifdef HAVE_FORK | ||
| /*[clinic input] | ||
| os.register_at_fork |
There was a problem hiding this comment.
Question: should this function be defined even when fork() isn't available?
There was a problem hiding this comment.
Maybe registering fork handlers require doing other preparation work. It is easy to test if os.register_at_fork exists, and this is the only way for testing if registering the fork handlers is needed or can be just skipped (with additional preparation work).
Modules/posixmodule.c
Outdated
| else if (!strcmp(when, "parent")) | ||
| lst = &interp->after_forkers_parent; | ||
| else { | ||
| PyErr_Format(PyExc_ValueError, "unexpected value for *when*: %R", |
There was a problem hiding this comment.
when is const char*, not PyObject*. I think you should use \"%s\" instead of %R.
It is unusual to use * for emphasizing/quoting in error message.
| /*[clinic input] | ||
| os.register_at_fork | ||
|
|
||
| func: object |
There was a problem hiding this comment.
Do you want to support passing all arguments by keyword? In that case I prefer the name "callable" for the first parameter.
atexit.register() has different signature and allows to pass arbitrary positional and keyword arguments to the registered function.
There was a problem hiding this comment.
Right, perhaps only when should be keyword-enabled.
There was a problem hiding this comment.
bikeshed: callable is the name of a built-in, I wouldn't use it as a parameter name. function perhaps.
There was a problem hiding this comment.
I kept func and made the first argument positional-only :-)
There was a problem hiding this comment.
Victor recently changed names of parameters of C API functions to "callable", but I see that most Python functions prefer the name "func". So now I prefer this name too.
Modules/posixmodule.c
Outdated
| void | ||
| PyOS_BeforeFork(void) | ||
| { | ||
| /* NOTE callbacks are prepended in register_at_fork(), which is |
There was a problem hiding this comment.
If register_at_fork() is called in a registered function, some registered functions can be called twice.
There was a problem hiding this comment.
That's true. I didn't want to bother about this edge case, but a simple solution would be to copy the list before processing it.
|
@1st1, were you planning to review this? |
Yes, LGTM from me. I'm going to use this API in uvloop (now I use |
|
My only question (that shouldn't block this PR from being merged) is can we use |
|
If fork() was called without the GIL had, the child process interpreter
could be in an unrecoverable non reentrant state.
…On Fri, May 26, 2017, 1:17 PM Yury Selivanov ***@***.***> wrote:
My only question (that shouldn't block this PR from being merged) is can
we use pthread_atfork in CPython? That would make this API work even when
a C extension is calling fork directly, bypassing the CPython API.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1715 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAELiymvfCsEjaXBQThXDfFlUB4sfiwkks5r9zNjgaJpZM4NiROU>
.
|
|
Thanks, Gregory, it makes sense. |
Resolve conflicts: 346cbd3 bpo-16500: Allow registering at-fork handlers (python#1715)
6 participants
No description provided.