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 — [3.7] bpo-6721: Hold logging locks across fork() (GH-4071) by miss-islington · Pull Request #9291 · python/cpython

Conversation

@miss-islington

@miss-islington miss-islington commented Sep 14, 2018 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

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]

https://bugs.python.org/issue6721

Sorry, something went wrong.

Uh oh!

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

All reactions
@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 added sprint type-bug An unexpected behavior, bug, or error labels Sep 14, 2018
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 14, 2018
@bedevere-bot bedevere-bot mentioned this pull request Sep 14, 2018
bpo-6721: Sanitize logging locks while forking #4071
Merged
@bedevere-bot bedevere-bot added the awaiting review label Sep 14, 2018
@gpshead

gpshead commented Sep 14, 2018

Copy link
Copy Markdown
Member

I'm letting this change "bake" in master for a bit, monitoring buildbots, before approving this 3.7 backport.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gpshead gpshead self-assigned this Sep 14, 2018
@miss-islington

miss-islington commented Sep 14, 2018

Copy link
Copy Markdown
Contributor Author

@gpshead: Status check is done, and it's a success ✅ .

All reactions

Sorry, something went wrong.

Uh oh!

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

2 similar comments
@miss-islington

miss-islington commented Sep 14, 2018

Copy link
Copy Markdown
Contributor Author

@gpshead: Status check is done, and it's a success ✅ .

All reactions

Sorry, something went wrong.

Uh oh!

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

@miss-islington

miss-islington commented Sep 14, 2018

Copy link
Copy Markdown
Contributor Author

@gpshead: Status check is done, and it's a success ✅ .

All reactions

Sorry, something went wrong.

Uh oh!

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

@ned-deily

ned-deily commented Oct 6, 2018

Copy link
Copy Markdown
Member

@gpshead Is this ready for 3.7?

All reactions

Sorry, something went wrong.

Uh oh!

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

@gpshead
gpshead merged commit 3b69993 into python:3.7 Oct 7, 2018
@bedevere-bot

bedevere-bot commented Oct 7, 2018

Copy link
Copy Markdown

@gpshead: Please replace # with GH- in the commit message next time. Thanks!

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 review label Oct 7, 2018
@miss-islington
miss-islington deleted the backport-1900384-3.7 branch October 7, 2018 07:10
@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

No reviews

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.

5 participants

@miss-islington @gpshead @ned-deily @bedevere-bot @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.