New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't call os.pid() the default kwargs to atexit_print_messages #3005

Merged
merged 1 commit into from May 31, 2016

Conversation

Projects
None yet
2 participants
@lamby
Contributor

lamby commented May 16, 2016

Whilst it has no side-effects, this results in non-reproducible output when
generating the certbot documentation:

http://i.imgur.com/Q6VGi9S.jpg

Signed-off-by: Chris Lamb chris@chris-lamb.co.uk

@pde pde assigned bmw May 16, 2016

@bmw

This comment has been minimized.

Show comment
Hide comment
@bmw

bmw May 27, 2016

Contributor

So this actually doesn't work, at least not in the way it was intended to.

This code was intended to fix the problem if the client forks. While it doesn't fork anymore (but might in the future), the problem we were having is we registered atexit_print_messages to run when the client exits. If the client forks, however, you'd get the messages printed multiple times because the messages would be printed for each exiting process.

Default arguments are evaluated when the function is defined. This allowed us to set the default value of pid to the pid of the process that first imported the module. This way, even though all exiting processes called atexit_print_messages, only the call that had the matching pid would actually print any messages.

The code proposed in this PR removes this protection though and messages will be printed for every exiting process. There are other solutions though. We could create a global variable or have atexit_print_messages register the call with atexit itself. In the latter approach, it could set internal state and use it to determine which process causes the messages to printed.

Hopefully this makes sense! Let me know if you have any questions.

Contributor

bmw commented May 27, 2016

So this actually doesn't work, at least not in the way it was intended to.

This code was intended to fix the problem if the client forks. While it doesn't fork anymore (but might in the future), the problem we were having is we registered atexit_print_messages to run when the client exits. If the client forks, however, you'd get the messages printed multiple times because the messages would be printed for each exiting process.

Default arguments are evaluated when the function is defined. This allowed us to set the default value of pid to the pid of the process that first imported the module. This way, even though all exiting processes called atexit_print_messages, only the call that had the matching pid would actually print any messages.

The code proposed in this PR removes this protection though and messages will be printed for every exiting process. There are other solutions though. We could create a global variable or have atexit_print_messages register the call with atexit itself. In the latter approach, it could set internal state and use it to determine which process causes the messages to printed.

Hopefully this makes sense! Let me know if you have any questions.

@bmw bmw removed their assignment May 27, 2016

@bmw bmw changed the title from Don't call os.pid() the default kwargs to atexit_print_messages to Don't call os.pid() the default kwargs to atexit_print_messages [needs revision] May 27, 2016

Don't call os.pid() the default kwargs to atexit_print_messages
Whilst it has no side-effects, this results in non-reproducible output when
generating the certbot documentation:

    http://i.imgur.com/Q6VGi9S.jpg

Signed-off-by: Chris Lamb <chris@chris-lamb.co.uk>
@lamby

This comment has been minimized.

Show comment
Hide comment
@lamby

lamby May 28, 2016

Contributor

this allowed us to set the default value of pid to the pid of the process that first imported the module.

Ahhhhh. :) Apologies for not spotting this but, on the other hand, it's the sort of thing that would definitely deserve a large/scary comment, if only to prevent someone helpfully following the instructions of a linter to remove dangerous-looking default arguments.

@bmw I've updated this PR to match and documented this behaviour explicitly.

Contributor

lamby commented May 28, 2016

this allowed us to set the default value of pid to the pid of the process that first imported the module.

Ahhhhh. :) Apologies for not spotting this but, on the other hand, it's the sort of thing that would definitely deserve a large/scary comment, if only to prevent someone helpfully following the instructions of a linter to remove dangerous-looking default arguments.

@bmw I've updated this PR to match and documented this behaviour explicitly.

@bmw bmw changed the title from Don't call os.pid() the default kwargs to atexit_print_messages [needs revision] to Don't call os.pid() the default kwargs to atexit_print_messages May 31, 2016

@bmw

This comment has been minimized.

Show comment
Hide comment
@bmw

bmw May 31, 2016

Contributor

Yeah. I agree there should have been a comment so thanks for adding one.

LGTM. Thanks @lamby.

Contributor

bmw commented May 31, 2016

Yeah. I agree there should have been a comment so thanks for adding one.

LGTM. Thanks @lamby.

@bmw bmw merged commit fcc4622 into certbot:master May 31, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment