GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-6721: Sanitize logging locks while forking by gpshead · Pull Request #4071 · python/cpython

Conversation

@gpshead

@gpshead gpshead commented Oct 21, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This part is simple, acquire and release the logging lock and each logging handler's lock around fork.

A unittest is still needed (relatively trivial to create).

The logging.handlers module has a QueueHandler and QueueListener which uses the queue module... that is full of locks. I'm not addressing that as I don't see an immediately obvious correct thing to do with that queue. A QueueListener thread won't be running in the forked child anyways so the right thing for anyone using one of those is likely to remove the QueueHandler from their logging config entirely... unless they go on to re-establish their complicated queue based logging handler setup in the child. suggestion: Do nothing. Let people using QueueHandler deal with their own problems as their application sees fit.

https://bugs.python.org/issue6721

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 21, 2017
@bedevere-bot bedevere-bot added the awaiting merge label Oct 21, 2017
pitrou
pitrou reviewed Oct 21, 2017
View reviewed changes
Comment thread Lib/logging/__init__.py Outdated
Acquire a thread lock for serializing access to the underlying I/O.
"""
self.lock = threading.RLock()
os.register_at_fork(before=self.acquire,

@pitrou pitrou Oct 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may register an arbitrary number of callbacks across an application's lifetime, also it makes the handles eternal by keeping a reference to all of them. Instead, we could have a global WeakSet of handlers and a single set of callbacks that iterate over the live handlers.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gpshead gpshead Sep 12, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option would be a before=lambda acquire=weakref.WeakMethod(self.acquire): acquire()()... I'm used to applications that don't go creating tons of logging.Handler instances and throwing them away all over the place. :)

I think I'll go with the WeakSet approach to avoid an every growing list of callables in that strange Handler discarding application case...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jakirkham jakirkham mentioned this pull request Jul 16, 2018
Default multiprocessing context is broken and should never be used dask/dask#3759
Closed
@ambv

ambv commented Aug 31, 2018

Copy link
Copy Markdown
Contributor

@gpshead Please come back to this patch during the sprint!
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead self-assigned this Sep 12, 2018
@gpshead
gpshead force-pushed the logging_locks_at_fork branch 2 times, most recently from 8d51698 to caa3e6f Compare September 13, 2018 08:41
gpshead added 2 commits September 13, 2018 10:03
@gpshead
[bpo-6721] Sanitize logging locks while forking.
64a53be
@gpshead
Use the WeakSet approach per code review.
a0c2cfb
@gpshead
gpshead force-pushed the logging_locks_at_fork branch from caa3e6f to a0c2cfb Compare September 13, 2018 17:03
@gpshead
blurb
1d5bbcb
@gpshead
gpshead requested a review from ambv September 13, 2018 17:10
@gpshead
Make the logic conditional on register_at_fork.
6be017a
@gpshead gpshead added type-bug An unexpected behavior, bug, or error sprint awaiting changes needs backport to 3.7 labels Sep 13, 2018
gpshead added 3 commits September 13, 2018 15:17
@gpshead
Add a unittest!
ebb4b74
@gpshead
Actually use the registration fn...
5d5426f
@gpshead
Fix the test to actually work.
1066bb1 
Now it fails on the previous code and is fixed by the logging changes.
The test now uses a thread to hold the locks during the fork in a
synchronized manner.

Due to mixing of fork and a thread there is potential for deadlock
in the child process.  buildbots and time will tell if this actually
manifests itself in this test or not.  :/
@gpshead

gpshead commented Sep 13, 2018

Copy link
Copy Markdown
Member Author

A regression test that actually catches the bug and is fixed by this change has been added.

Due to mixing of fork and a thread there is potential for deadlock
in the child process. buildbots and time will tell if this actually
manifests itself in this test or not. :/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead removed the awaiting changes label Sep 14, 2018
@gpshead
gpshead merged commit 1900384 into python:master Sep 14, 2018
@miss-islington

miss-islington commented Sep 14, 2018

Copy link
Copy Markdown
Contributor

Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 14, 2018
@gpshead
gpshead deleted the logging_locks_at_fork branch September 14, 2018 05:08
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 14, 2018
@gpshead @miss-islington
bpo-6721: Hold logging locks across fork() (pythonGH-4071)
8f01014 
bpo-6721: When os.fork() was called while another thread holds a logging lock, the child process may deadlock when it tries to log.  This fixes that by acquiring all logging locks before fork and releasing them afterwards.

A regression test that fails before this change is included.

Within the new unittest itself: There is a small _potential_ due to mixing of fork and a thread in the child process if the parent's thread happened to hold a non-reentrant library call lock (malloc?) when the os.fork() happens.  buildbots and time will tell if this actually manifests itself in this test or not.  :/  A functionality test that avoids that would be a challenge.

An alternate test that isn't trying to produce the deadlock itself but just checking that the release and acquire calls are made would be the next best alternative if so.
(cherry picked from commit 1900384)

Co-authored-by: Gregory P. Smith <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.7 label Sep 14, 2018
@bedevere-bot

bedevere-bot commented Sep 14, 2018

Copy link
Copy Markdown

GH-9291 is a backport of this pull request to the 3.7 branch.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead pushed a commit that referenced this pull request Oct 7, 2018
@miss-islington @gpshead
bpo-6721: Hold logging locks across fork() (GH-4071) (#9291)
3b69993 
bpo-6721: When os.fork() was called while another thread holds a logging lock, the child process may deadlock when it tries to log.  This fixes that by acquiring all logging locks before fork and releasing them afterwards.

A regression test that fails before this change is included.

Within the new unittest itself: There is a small _potential_ due to mixing of fork and a thread in the child process if the parent's thread happened to hold a non-reentrant library call lock (malloc?) when the os.fork() happens.  buildbots and time will tell if this actually manifests itself in this test or not.  :/  A functionality test that avoids that would be a challenge.

An alternate test that isn't trying to produce the deadlock itself but just checking that the release and acquire calls are made would be the next best alternative if so.
(cherry picked from commit 1900384)

Co-authored-by: Gregory P. Smith <[email protected]> [Google]
@hroncok

hroncok commented Nov 6, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Just a heads up: We have reverted this commit in Fedora's Python package (version 3.7.1), because it rendered our graphical installers not bootable. This may have just exposed some bug in the installer or it may have introduced a regression. We are currently investigating the problem with Fedora QA and the installer developers and we will open a BPO issue if we find out a simple reproducer.

Details: https://bugzilla.redhat.com/show_bug.cgi?id=1644936
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tomkooij tomkooij mentioned this pull request Jan 9, 2019
Using multiple threads results in deadlock of the hisparc-update (updatehistograms job) HiSPARC/publicdb#242
Closed
@gpshead gpshead mentioned this pull request Apr 10, 2022
Locks in the standard library should be sanitized on fork #50970
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitrou pitrou pitrou left review comments
@ambv ambv Awaiting requested review from ambv

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

sprint type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@gpshead @ambv @miss-islington @bedevere-bot @hroncok @pitrou @the-knights-who-say-ni
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.