bpo-16500: Don't use string constants for os.register_at_fork() behavior by gpshead · Pull Request #1834 · python/cpython

gpshead · 2017-05-27T18:40:16Z

Add os module constants and updates all uses and documentation instead of string literals.
This allows for cleaner code with less chance of a typo in a string value showing up at runtime.
Linters and similar tools can flag incorrect constant names before run time.

pitrou · 2017-05-27T18:55:50Z

I'm not sure I like this. This is longer to type, and doesn't seem to bring anything over human-readable string constants (compare with, say, SEET_SET and friends which are opaque integer values). If you make a typo, since os.register_at_fork will typically be called at module import, that typo will probably show up immediately.

@1st1 ?

gpshead · 2017-05-27T19:16:07Z

I wouldn't expect register_at_fork() to be called at import time in most non-stdlib use cases. An application is more likely to call it later as it starts running things that need special treatment (especially for crazy developers who refuse to learn that mixing fork and threads is a bad idea).

My main complaint is the use of magic string values.

Alternatives would be

To make three functions instead of having a when parameter.
Or to mirror pthread_atfork()'s API and accept three callables (keyword args only at that point). I kind of like that idea. A single register_at_fork() function, with three optional named parameters.

pitrou · 2017-05-27T19:25:49Z

Well, I would rather have constants than your two other suggestions :-)

gpshead · 2017-05-27T19:33:17Z

os.register_at_fork(before=prepare_for_launch,
                    after_parent=launch_cleanup,
                    after_child=reinitialize_stuff)

Looks very readable to me.

pitrou · 2017-05-27T19:34:10Z

Why not. It's much better than having three separate functions, at least.

serhiy-storchaka · 2017-05-28T08:30:50Z

Is "if control is expected to return to the Python interpreter" related to other arguments? _posixsubprocess.fork_exec() doesn't call any of handlers if preexec_fn is None.

And the same question about the documentation of PyOS_AfterFork_Child() vs other functions.

serhiy-storchaka · 2017-05-28T08:33:06Z

It is not clear what argument triggered an error. Use three different invocations with three assertRaises.

serhiy-storchaka · 2017-05-28T08:33:34Z

The same as above.

serhiy-storchaka · 2017-05-28T08:36:10Z

Double backticks don't look nice in text output.

serhiy-storchaka · 2017-05-28T08:46:54Z

This means that os.register_at_fork(before=lambda:None, after_in_child=42, after_in_parent='spam') is successful.

I would change this condition to just (!(before || after_in_child || after_in_parent)) or (!before && !after_in_child && !after_in_parent) and add separate PyCallable_Check() checks for every non-NULL argument.

Since this case was not caught by tests, needed a new test for it.

serhiy-storchaka · 2017-05-28T08:51:28Z

The check for before != NULL can be moved into register_at_forker() together with a PyCallable_Check() check. Unless you want to produce more specific error messages.

serhiy-storchaka · 2017-05-28T08:53:37Z

HAVE_FORK is internal CPython define. It is not a part of any API.

1st1 · 2017-05-28T18:07:03Z

Having linters and IDEs helping you with the register_at_fork signature is a good idea, although I still like the original API proposed by @pitrou.

+1 to go with keyword arguments, instead of constants.

os.register_at_fork(before=prepare_for_launch,
                    after_parent=launch_cleanup,
                    after_child=reinitialize_stuff)

gpshead · 2017-05-28T19:41:03Z

Thanks @serhiy-storchaka I believe I've addressed all of those suggestions.

Adds the constants and updates all uses and documentation.

This switches from using str constants or os module constants on a warn parameter to using named keyword only arguments specifying the intent of the callable being passed in. It avoids the need for string literals or new os module constants and is generally more readable. This is more similar to how the libc pthread_atfork() API works.

We don't want to call PyOS_BeforeFork() or PyOS_AfterFork_Parent() if we never manage to make it to the fork() call it self, erroring out instead.

The statement about re-entering the interpreter applies to all three calls, not just after_in_child.

serhiy-storchaka · 2017-05-28T20:06:04Z

The error message looks like:

'before' must be callable, not <class 'str'>

It is more common to include the type name rather than repr.

'before' must be callable, not str

%s and Py_TYPE(obj)->tp_name solves that. thanks.

serhiy-storchaka · 2017-05-28T20:08:32Z

It is more common to return either

1 on success, 0 on failure, or

0 on success, -1 on failure.

it doesn't really matter. 0 on success and non-zero on failure is the real idiom. 1 and -1 are both non-zero and pass a truth test.

serhiy-storchaka · 2017-05-28T20:11:43Z

This partially repeats descriptions of arguments ('before' is called before forking, etc).

I removed some of the redundancy in the docstring.

serhiy-storchaka · 2017-05-28T20:12:23Z

"in the parent" perhaps is redundant.

And "Zero arg" may be redundant in the docstring. This detail can be explained in the module documentation. Shortened "arg" is a jargon word.

serhiy-storchaka · 2017-05-28T20:20:42Z

serhiy-storchaka · 2017-05-28T20:23:36Z

os.fork() is not the only forking function. There are also os.fork1(), os.forkpty() and _posixsubprocess.fork_exec(). And third-party code can call fork().

It is impossible and unmaintainable to list them all, I've generalized the wording.

serhiy-storchaka · 2017-05-28T21:03:00Z

I don't know why, but the diff now shows already merged changes, including unrelated to this issue. This makes reviewing hard.

gpshead · 2017-05-28T22:36:39Z

I don't know why git did that. I followed the https://cpython-devguide.readthedocs.io/gitbootcamp.html#syncing-with-upstream instructions to pull in recent changes so the tests would pass and I could update Lib/threading.py to use this API. :(

gpshead · 2017-05-28T22:54:24Z

Okay, I figured out how to do a proper rebasing and the diffs for this PR look sane again without including merged changes from others.

(for reference, the git instructions I followed were based on https://www.digitalocean.com/community/tutorials/how-to-rebase-and-update-a-pull-request)

pitrou · 2017-05-29T10:09:17Z

 "--\n"
 "\n"
-"Register a callable object to be called when forking.\n"
+"Registers callables to be called when forking a new process.\n"


As a style rule, CPython uses infinitives in documentation, not present tense.

pitrou · 2017-05-29T10:12:48Z

btw, if you don't want to mess around with git rebases, git pull origin master should be sufficient to merge the latest changes from master into your current branch (replace "origin" with whatever the upstream CPython is named in your working clone).

I believe all comments have been addressed. Anything else can be addressed in follow up commits.

gpshead self-assigned this May 27, 2017

gpshead changed the title ~~bpo-16500: Use constants for os.register_at_fork()~~ bpo-16500: Don't use string constants for os.register_at_fork() behavior May 28, 2017

gpshead requested a review from serhiy-storchaka May 28, 2017 08:00

serhiy-storchaka previously requested changes May 28, 2017

View reviewed changes

gpshead added 10 commits May 28, 2017 13:20

Use os module constants for os.register_at_fork.

f638efa

Adds the constants and updates all uses and documentation.

Undo the unnecessary blank line os.py change.

50506fc

Fix a docstring typo in posixmodule.

389b767

Don't mention the HAVE_FORK cpp #define.

36525ce

Don't use backticks in the docstring.

6ce5170

bpo-16500, don't call forkers if we never forked().

1f07b5c

We don't want to call PyOS_BeforeFork() or PyOS_AfterFork_Parent() if we never manage to make it to the fork() call it self, erroring out instead.

bpo-16500: Clarify when all three calls are made.

c51902b

The statement about re-entering the interpreter applies to all three calls, not just after_in_child.

Improve the test cases to cover the required logic.

3ee495d

fix register_at_fork parameter validation.

539c285

serhiy-storchaka reviewed May 28, 2017

View reviewed changes

gpshead added 3 commits May 28, 2017 15:49

update threading to use the new API.

81597a6

Documentation refactoring based on code review.

52db2af

Use the type name rather than repr in the error.

22a40c9

gpshead force-pushed the atfork-when-cleanup-16500 branch from 9c9dbac to 22a40c9 Compare May 28, 2017 22:52

pitrou reviewed May 29, 2017

View reviewed changes

Use the infinitive in the docstring.

5132797

gpshead merged commit 163468a into python:master May 29, 2017

gpshead deleted the atfork-when-cleanup-16500 branch May 29, 2017 17:04

Uh oh!

Conversation

gpshead commented May 27, 2017

Uh oh!

pitrou commented May 27, 2017

Uh oh!

gpshead commented May 27, 2017

Uh oh!

pitrou commented May 27, 2017

Uh oh!

gpshead commented May 27, 2017

Uh oh!

pitrou commented May 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented May 28, 2017

Uh oh!

gpshead commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented May 28, 2017

Uh oh!

gpshead commented May 28, 2017

Uh oh!

gpshead commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented May 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants