gh-101819: Isolate _io by erlend-aasland · Pull Request #101948 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation44 Commits86 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
Also remove calls to _PyIOBase_finalize from dealloc funcs
🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit ab1baf4 🤖
If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.
I am not sure how it is not needed. I see similar thing done in
_csvmodule.
Hm, Serhiy's general fix (gh-22870) was committed just before the _csv extension module was adapted (gh-23224).
When I try pickling with protocols 0 and 1 in the REPL, I get a correct TypeError for the _io types. It's strange that this is not the behaviour we're also seeing in test_pickling.
When I try pickling with protocols 0 and 1 in the REPL, I get a correct TypeError for the _io types. It's strange that this is not the behaviour we're also seeing in test_pickling.
Did you remove the _PyIOBase_cannot_pickle before doing that? I tried and it pickled without error. Anyways I am not really an expert on pickle.
Did you remove the
_PyIOBase_cannot_picklebefore doing that? I tried and it pickled without error. Anyways I am not really an expert on pickle.
Probably not; I tried again yesterday, and it also pickled without error. I'm not an expert on pickle either, but it seems to me we've found a corner case where Serhiy's fix of 2020 does not apply. One option can be to fix Lib/copyreg.py. In any case, I suggest to land this PR with the pickle workaround in it. If we end up patching Serhiy's general pickle fix, we can revert the workaround introduced here (as Serhiy's patch did with other extension modules back in 2020).
Agreed, I'll make a final review and merge by later today.
root@codespaces-62bef3 /w/cpython (isolate-io/poc) [SIGINT]# ./python -m test -R 3:3 test_io 0:00:00 load avg: 7.01 Run tests sequentially 0:00:00 load avg: 7.01 [1/1] test_io beginning 6 repetitions 123456 ...... test_io passed in 3 min 31 sec
== Tests result: SUCCESS ==
1 test OK.
Total duration: 3 min 31 sec Tests result: SUCCESS
carljm added a commit to carljm/cpython that referenced this pull request
carljm added a commit to carljm/cpython that referenced this pull request
It's good to see _PyIO_get_module_state() going away :-) Also, bye bye my static_types and _PyIO_FiniTypes() workarounds!
I'm happy that these changes were merged as a long list of changes (see PR list in issue #101819): if something will go wrong, it will be easier to identify which sub-part of these changes is causing troubles. I expect troubles, but that's fine. We already have to go trough turbulences to modernize Python ;-)
Thanks @erlend-aasland and @kumaraditya303 for your hard work on this complicated _io extension.
🤖 New build scheduled with the buildbot fleet by @sunmy2019 for commit fe2db1b 🤖
If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.
@sunmy2019, why did you engage the refleak bots on a closed PR? Did you check the results of the refleak run that Kumar did for the same commit (fe2db1b) yesterday first? (All 31 checks passed.)
It should not pass the ref leaks tests. #104510 Just rerun to check what's going on.
Also, I should not trigger builtbot on a merged PR. It seems nothing will be tested. That's my bad.
Also, I should not trigger builtbot on a merged PR. It seems nothing will be tested. That's my bad.
Yes. And you can examine the refleak run Kumar did post merge. I did yesterday. I'm not sure why that passed, though.
Yes. And you can examine the refleak run Kumar did post merge. I did yesterday. I'm not sure why that passed, though.
Your PR was created in February. Maybe the code evolved in the meanwhile and the final "squash + rebase" commit is different than the PR branch ran on buildbots. For such PR which is in the works for a long time, I prefer to manually squash+rebase time to time to workaround this workflow limitation. There are services like https://mergify.com/ which prevent this workflow flaw. I heard that GitHub automerge can do something similar (run GHA jobs on the "final" commit), but buildbots are not handled and use the old way (run the jobs on the PR branch which is not merged into main / rebased).
musicinmybrain added a commit to musicinmybrain/indexed_gzip that referenced this pull request
Restores “pickle-ability” of IndexedGzipFile in Python 3.12, which was broken due to python/cpython#101948.
musicinmybrain added a commit to musicinmybrain/indexed_gzip that referenced this pull request