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

ffi: add PyConfig::warn_default_encoding to 3.10+ #2370

Closed
wants to merge 2 commits into from

Conversation

indygreg
Copy link
Contributor

This field was added in 3.10 but we missed it. This was causing
offsets of subsequent fields to be wrong. This could lead to
unexpected behavior or even crashes.

This bug is causing many PyOxidizer built binaries to crash when using 3.10.

@indygreg indygreg force-pushed the initconfig-310-compat branch 2 times, most recently from 036eb76 to 4c6d182 Compare May 12, 2022 01:49
@indygreg
Copy link
Contributor Author

I confirmed that this patch fixes the crashes that occur with PyOxidizer when using 3.10.

CHANGELOG.md Outdated
@@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- Fixed incorrectly disabled FFI definition `PyThreadState_DeleteCurrent`. [#2357](https://github.com/PyO3/pyo3/pull/2357)
- Correct FFI definition `PyEval_EvalCodeEx` to take `*const *mut PyObject` array arguments instead of `*mut *mut PyObject` (this was changed in CPython 3.6). [#2368](https://github.com/PyO3/pyo3/pull/2368)
- Added missing `warn_default_encoding` field to `PyConfig` on 3.10+. [#2370](https://github.com/PyO3/pyo3/pull/2370)
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to mention the symptom - PyOxidizer crashes - here so that people experiencing them can match that to the changelog.

@davidhewitt
Copy link
Member

Argh, I'm sorry we missed this. Given that 0.17 release is probably still a little way away (got a few unfinished bits merged), we should perhaps backport this to 0.16?

@davidhewitt
Copy link
Member

A passing thought - would you be willing to add a new file test_pep_587 which tests spinning up an interpreter using these APIs? That should help avoid this happening again if anything changes in the future.

@indygreg
Copy link
Contributor Author

Argh, I'm sorry we missed this. Given that 0.17 release is probably still a little way away (got a few unfinished bits merged), we should perhaps backport this to 0.16?

I would strongly prefer a backport and 0.16.x release at your earliest convenience.

A passing thought - would you be willing to add a new file test_pep_587 which tests spinning up an interpreter using these APIs? That should help avoid this happening again if anything changes in the future.

If it isn't too much work, yes. However, I'm unsure the state of your testing infra. These interpreter-level config options result in global variables getting set in CPython and CPython - especially older versions - isn't great about ensuring all values are normalized during interpreter initialization or finalization. For equivalent tests I already have in the pyembed crate, I need to run each test in its own process to prevent state leakage between tests. This gets thorny, especially on Windows. But if you already have an easy way to spin up new interpreters in new processes in tests, it shouldn't be difficult to implement test coverage.

@davidhewitt
Copy link
Member

👍 I'll do my best to find time to push this out on Saturday.

But if you already have an easy way to spin up new interpreters in new processes in tests, it shouldn't be difficult to implement test coverage.

My understand was that cargo runs each test file from the tests/ subdirectory in a new process? If so, adding a new file with a single test should be sufficient. I was hoping you have something trivial from pyembed you might be able to strip down and repurpose as a test.

@indygreg
Copy link
Contributor Author

👍 I'll do my best to find time to push this out on Saturday.

Thank you!

But if you already have an easy way to spin up new interpreters in new processes in tests, it shouldn't be difficult to implement test coverage.

My understand was that cargo runs each test file from the tests/ subdirectory in a new process? If so, adding a new file with a single test should be sufficient. I was hoping you have something trivial from pyembed you might be able to strip down and repurpose as a test.

Yes, I think that's how Cargo works. But I was hoping to avoid separate files because the repetition gets tedious. For pyembed, I use rusty_fork, which has a macro for spawning a test function in a new process. There are some rough edges getting shared libraries to all resolve properly in the context of the test process. But I have it working. See https://github.com/indygreg/PyOxidizer/blob/7b21c2b950b21e9d39002703c5dcc45a6280b73a/pyembed/src/test/interpreter_config.rs for my existing tests.

@indygreg
Copy link
Contributor Author

It also looks like pathconfig_warnings and program_name have the wrong order on 3.10. I'll be amending this PR accordingly.

I'll likely also send a PR to add 3.11 changes to initconfig.h. But I don't expect or need this to be backported to 0.16.

This field was added in 3.10 but we missed it. This was causing
offsets of subsequent fields to be wrong. This could lead to
unexpected behavior or even crashes.
@davidhewitt
Copy link
Member

Thanks! I've managed this evening to push a regression test to this branch which should help ensure things keep working in the future (I could verify it failed on main and passes with this branch). I'll finish up and release in the morning.

@davidhewitt
Copy link
Member

Aha perfect, the test now fails on 3.11 alpha which I guess is expected! For speed I'll just merge #2371 which has both your commits and then verify with a new MR for the test.

indygreg added a commit to indygreg/PyOxidizer that referenced this pull request May 15, 2022
This fixes a nasty bug on Python 3.10 leading to runtime crashes
and wrong behavior. (PyO3/pyo3#2370)
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.

None yet

3 participants