Skip to content

[WIP] bpo-40091: Fix a crash in logging after fork #19196

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

Closed
wants to merge 1 commit into from
Closed

[WIP] bpo-40091: Fix a crash in logging after fork #19196

wants to merge 1 commit into from

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Mar 27, 2020

Fix a crash in the logging module after fork in the process process:
reset the main logging lock.

https://bugs.python.org/issue40091

Fix a crash in the logging module after fork in the process process:
reset the main logging lock.
@vstinner
Copy link
Member Author

@vsajip, @gpshead: Would you mind to review this change?

@vstinner
Copy link
Member Author

vstinner commented Mar 27, 2020

I wrote this change to be able to backport it to stable 3.7 and 3.8 branches.

When bpo-40089 will be fixed, we can enhance the _after_at_fork_child_reinit_locks() in the master branch to use the new _at_fork_reinit() method.

@vstinner
Copy link
Member Author

Oh. A colleague working on the glibc tells me that it's unsafe to release the memory of the old lock without destroying it: "Putting two mutexes at the same address could lead to problems".

We should either leak memory or destroy the old lock.

To destroy the old lock, we need to first unlock it if it's lock. pthread_mutex_trylock() can be used to check if it's locked or not. If it's lock, call pthread_mutex_unlock(). Then pthread_mutex_destroy() and pthread_mutex_init().

@vstinner
Copy link
Member Author

Sadly, test_manager_initializer() of test_multiprocessing_forkserver hangs with this change :-(

Maybe I should wait for https://bugs.python.org/issue40089 to be fixed first.

Replacing _releaseLock() with _lock._at_fork_reinit() works as expected: test_manager_initializer() doesn't hang.

@vstinner vstinner changed the title bpo-40091: Fix a crash in logging after fork [WIP] bpo-40091: Fix a crash in logging after fork Mar 28, 2020
@vstinner
Copy link
Member Author

vstinner commented Apr 7, 2020

This approach doesn't work. Since I merged my PR #19195 adding _at_fork_reinit() method to locks, I abandon this PR in favor of PR #19416.

@vstinner vstinner closed this Apr 7, 2020
@vstinner vstinner deleted the logging_fork branch April 7, 2020 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants