Skip to content
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

gil: tidy ups to finalization #1355

Merged
merged 2 commits into from
Jan 12, 2021
Merged

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Jan 3, 2021

EDIT: I changed strategy. See #1355 (comment) for the latest design.

This is a follow-up to #1347 which adds some tidy-ups to our finalization behavior:

  • Calling Py_Finalize in a different thread to Py_Initialize will cause an AssertionError. (See https://bugs.python.org/issue26693)

    So I changed prepare_freethreaded_python to initialize and finalize in a dedicated thread.

  • We have some C-extensions which have crashes with Py_Finalize. So after disabling the auto-initialize feature we can ask users with problematic extensions to call the new prepare_freethreaded_python_without_finalizer to workaround these. I added this to the FAQ.

Closes #1073
Closes #1261

@davidhewitt
Copy link
Member Author

Hmm, the coverage job seems to have some kind of deadlock...

@davidhewitt davidhewitt force-pushed the finalization branch 3 times, most recently from 559125b to cbfb8c4 Compare January 6, 2021 09:04
@davidhewitt
Copy link
Member Author

@kngwyu this PR is now ready for review - would be interested to know what you think of it.

@kngwyu
Copy link
Member

kngwyu commented Jan 6, 2021

So I changed prepare_freethreaded_python to initialize and finalize in a dedicated thread.

If we really need this trick I don't want to use libc::atexit to call finalize. Is there any way to call finalize from the main thread?

@kngwyu
Copy link
Member

kngwyu commented Jan 6, 2021

I think it's also good to provide #[pyo3::main] fn main() { .. }, but am not sure about which approach is better.

@davidhewitt
Copy link
Member Author

If we really need this trick I don't want to use libc::atexit to call finalize. Is there any way to call finalize from the main thread?

The problem is in tests there is no main thread, so we will always need some kind of trick like this for tests.

#[pyo3::main(finalize = false)] is a really nice idea; I guess we will build it on top of this API. I'll push a commit to experiment with that later.

@kngwyu
Copy link
Member

kngwyu commented Jan 7, 2021

Then how about memorizing the thread id and calling finalize if the current thread == initialized thread?

Also, I think we need to rethink the use cases of Py_Finalize.

The document notes some examples:

This function is provided for a number of reasons. An embedding application might want to restart Python without having to restart the application itself. An application that has loaded the Python interpreter from a dynamically loadable library (or DLL) might want to free all memory allocated by Python before unloading the DLL. During a hunt for memory leaks in an application a developer might want to free all memory allocated by Python before exiting from the application.

Our original motivation (clearing the buffer) does not match any of these, and I'm kind of doubtful about the idea that it must be called.

@davidhewitt
Copy link
Member Author

I think I don't want to do anything like memorize the thread id, as then it's unpredictable in tests whether finalize is called, which doesn't seem good for reproducability. Also I'm now thinking that because we know lots of crashes can occur at finalization it's better not to have it as a default.

I pushed a new commit which removes atexit completely from prepare_freethreaded_python and adds a new api with_embedded_python_interpreter which does both initialization and finalization guaranteed to be on the current thread.

We could implement #[pyo3::main] and #[pyo3::main(finalize = true)] (opt-in finalization) on top of these APIs trivially as a future PR.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks!
This is a breaking change, but I believe it would be applicable.

src/gil.rs Outdated Show resolved Hide resolved
Comment on lines +140 to +147
assert_eq!(
ffi::Py_IsInitialized(),
0,
"called `with_embedded_python_interpreter` but a Python interpreter is already running."
);
Copy link
Member

Choose a reason for hiding this comment

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

If we force some error type (e.g., PyErr), we can return an Err here. It's an optional choice, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think PyErr doesn't work here because then the user will have a PyErr outside of the scope of a Python interpreter!

The hazards of unsafe 😅

src/gil.rs Outdated Show resolved Hide resolved
@davidhewitt davidhewitt force-pushed the finalization branch 3 times, most recently from 94b8224 to bc6a37a Compare January 11, 2021 22:16
@davidhewitt
Copy link
Member Author

@kngwyu this PR is ready for another look. I'm quite happy with it now, would be great to hear if you think there should be any further changes.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Importing SciPy causes segfault in PyFinalize() Importing tensorflow causes a crash on exit
2 participants