bpo-16500: Allow registering at-fork handlers by pitrou · Pull Request #1715 · python/cpython
| PyAPI_FUNC(void) PyOS_InitInterrupts(void); | ||
| PyAPI_FUNC(void) PyOS_AfterFork(void); | ||
| #ifdef HAVE_FORK | ||
| PyAPI_FUNC(void) PyOS_BeforeFork(void); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: should these functions be defined even when fork() isn't available?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They can be used only by the code that calls fork() (or similar functions).
|
|
||
| #ifdef HAVE_FORK | ||
| /*[clinic input] | ||
| os.register_at_fork |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: should this function be defined even when fork() isn't available?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
| else if (!strcmp(when, "parent")) | ||
| lst = &interp->after_forkers_parent; | ||
| else { | ||
| PyErr_Format(PyExc_ValueError, "unexpected value for *when*: %R", |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
| /*[clinic input] | ||
| os.register_at_fork | ||
|
|
||
| func: object |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, perhaps only when should be keyword-enabled.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bikeshed: callable is the name of a built-in, I wouldn't use it as a parameter name. function perhaps.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept func and made the first argument positional-only :-)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| void | ||
| PyOS_BeforeFork(void) | ||
| { | ||
| /* NOTE callbacks are prepended in register_at_fork(), which is |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If register_at_fork() is called in a registered function, some registered functions can be called twice.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
1st1
approved these changes
@1st1, were you planning to review this?
Yes, LGTM from me. I'm going to use this API in uvloop (now I use pthread_atfork) and I wanted to make sure that my use case is covered. And it is! Thanks for working on this.
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.
If fork() was called without the GIL had, the child process interpreter could be in an unrecoverable non reentrant state.
…
Thanks, Gregory, it makes sense.
ma8ma added a commit to ma8ma/cpython that referenced this pull request
Resolve conflicts: 346cbd3 bpo-16500: Allow registering at-fork handlers (python#1715)