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

bpo-1635741 - try to port pickle to multi-stage init #21319

Closed
wants to merge 1 commit into from

Conversation

koubaa
Copy link
Contributor

@koubaa koubaa commented Jul 4, 2020

Right now this patch causes a segfault:

test_unpickle_module_race (test.test_pickle.PyUnpicklerTests) ... Segmentation fault

It's really a clever test. Anyways hopefully the cause is apparent to reviewers. Otherwise I can try to debug it on my own some more

https://bugs.python.org/issue1635741

@koubaa
Copy link
Contributor Author

koubaa commented Jul 5, 2020

Found the problem. PyState_FindModule is used heavily (via _Pickle_GetGlobalState) and this is only supported for single phase init. I could replace some of those with PyType_GetModuleState but some types use Py_TPFLAGS_BASETYPE which means it may not be corrent in one for subclasses.

@vstinner Does PEP 573 (or a subsequent PEP) offer any alternative to that?

@vstinner
Copy link
Member

vstinner commented Jul 7, 2020

@vstinner Does PEP 573 (or a subsequent PEP) offer any alternative to that?

Not yet. Until PEP 573 is completed, I suggest to leave extensions which require this feature unmodified. So I suggest to close this PR for now, for example.

cc @corona10 @shihai1991 @encukou

@koubaa koubaa closed this Jul 7, 2020
@shihai1991
Copy link
Member

ps: you can attention petr's thread if you have free time: https://mail.python.org/archives/list/capi-sig@python.org/thread/T3P2QNLNLBRFHWSKYSTPMVEIL2EEKFJU/

@koubaa koubaa deleted the bpo-1635741-pickle branch August 28, 2020 00:20
@koubaa koubaa restored the bpo-1635741-pickle branch September 7, 2020 19:18
@koubaa koubaa deleted the bpo-1635741-pickle branch November 4, 2020 03:21
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.

5 participants