Skip to content

bpo-46417: Finalize structseq types at exit#30645

Merged
vstinner merged 5 commits intopython:mainfrom
vstinner:WIP_type_fini
Jan 21, 2022
Merged

bpo-46417: Finalize structseq types at exit#30645
vstinner merged 5 commits intopython:mainfrom
vstinner:WIP_type_fini

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Jan 17, 2022 •
edited
Loading

Add _PyStructSequence_FiniType() and _PyStaticType_Dealloc()
functions to finalize a structseq static type in Py_Finalize().
Currrently, these functions do nothing if Python is built in release
mode.

Clear static types:

  • AsyncGenHooksType: sys.set_asyncgen_hooks()
  • FlagsType: sys.flags
  • FloatInfoType: sys.float_info
  • Hash_InfoType: sys.hash_info
  • Int_InfoType: sys.int_info
  • ThreadInfoType: sys.thread_info
  • UnraisableHookArgsType: sys.unraisablehook
  • VersionInfoType: sys.version
  • WindowsVersionType: sys.getwindowsversion()

https://bugs.python.org/issue46417

@vstinner
Copy link
Member Author

vstinner commented Jan 17, 2022 •
edited
Loading

There are a few more structseq types:

  • Modules/_cursesmodule.c: NcursesVersionType
  • Modules/_threadmodule.c: ExceptHookArgsType
  • Modules/signalmodule.c: SiginfoType
  • Modules/timemodule.c: StructTimeType
  • Objects/longobject.c: Int_InfoType <= DONE
  • Python/errors.c: UnraisableHookArgsType <= DONE
  • Python/sysmodule.c: FlagsType <= DONE
  • Python/sysmodule.c: VersionInfoType <= DONE
  • Python/sysmodule.c: WindowsVersionType <= DONE
  • Python/thread.c: ThreadInfoType <= DONE

@vstinner
Copy link
Member Author

vstinner commented Jan 17, 2022

cc @erlend-aasland @corona10

shihai1991
shihai1991 reviewed Jan 19, 2022
View reviewed changes
@vstinner
Copy link
Member Author

vstinner commented Jan 19, 2022

I changed my mind and also enabled the code on Python release build. Previously, I only enabled the code if the Py_DEBUG macro was defined. For example, the Windows CI job on pull requests builds Python in release mode. If something is wrong, I prefer to catch the bug early and fix it.

@vstinner
bpo-46417: Finalize structseq types at exit
9987879 
Add _PyStructSequence_FiniType() and _PyStaticType_Dealloc()
functions to finalize a structseq static type in Py_Finalize().
Currrently, these functions do nothing if Python is built in release
mode.

Clear static types:

* AsyncGenHooksType: sys.set_asyncgen_hooks()
* FlagsType: sys.flags
* FloatInfoType: sys.float_info
* Hash_InfoType: sys.hash_info
* Int_InfoType: sys.int_info
* ThreadInfoType: sys.thread_info
* UnraisableHookArgsType: sys.unraisablehook
* VersionInfoType: sys.version
* WindowsVersionType: sys.getwindowsversion()
@vstinner vstinner changed the title bpo-46417: Finalize a few structseq types at exit bpo-46417: Finalize structseq types at exit Jan 19, 2022
@vstinner vstinner marked this pull request as ready for review January 19, 2022 16:09
@vstinner vstinner requested a review from markshannon as a code owner January 19, 2022 16:09
@vstinner
Copy link
Member Author

vstinner commented Jan 19, 2022

The PR is now ready for a review :-)

shihai1991
shihai1991 approved these changes Jan 20, 2022
View reviewed changes
Copy link
Member

@shihai1991 shihai1991 left a comment

Choose a reason for hiding this comment

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

OK. This PR looks good to me :)
But I think another question: Can we add a global cache to maintain the StructSequenceType and don't call the dealloc function in finalize_interp_types() manually?

@vstinner
Copy link
Member Author

vstinner commented Jan 20, 2022

Can we add a global cache to maintain the StructSequenceType and don't call the dealloc function in finalize_interp_types() manually?

Do you mean to create a list of structseq types when _PyStructSequence_InitType() or _PyStructSequence_InitType2() is called? Yeah, I had this idea, but I am not sure about it.

A problem is to control exactly when these types are deleted and types them in a specific order. The Python finalization is quite complex, see my notes about past issues: https://pythondev.readthedocs.io/finalization.html

assert(Py_REFCNT(type) == 1); of _PyStructSequence_FiniType() fails if something still keeps the a reference to the type somewhere: if the type is deallocated "too early".

@shihai1991
Copy link
Member

shihai1991 commented Jan 20, 2022

Can we add a global cache to maintain the StructSequenceType and don't call the dealloc function in finalize_interp_types() manually?

Do you mean to create a list of structseq types when _PyStructSequence_InitType() or _PyStructSequence_InitType2() is called? Yeah, I had this idea, but I am not sure about it.

Yes. That's what I mean : )

A problem is to control exactly when these types are deleted and types them in a specific order. The Python finalization is quite complex, see my notes about past issues: https://pythondev.readthedocs.io/finalization.html

assert(Py_REFCNT(type) == 1); of _PyStructSequence_FiniType() fails if something still keeps the a reference to the type somewhere: if the type is deallocated "too early".

OK, Thanks for your information. I will study your notes.

corona10
corona10 approved these changes Jan 20, 2022
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

I am +1 on this change :)

but same as @encukou, we may need tests to sure if the types are correctly reinitialized if there are multiple finalize/init cycles

@vstinner
Copy link
Member Author

vstinner commented Jan 20, 2022

@corona10:

but same as @encukou, we may need tests to sure if the types are correctly reinitialized if there are multiple finalize/init cycles

I added tests. Would you mind to review these tests as well?

@vstinner
Copy link
Member Author

vstinner commented Jan 20, 2022

I enhanced the tests to check more PyTypeObject members.

@vstinner vstinner merged commit e9e3eab into python:main Jan 21, 2022
@vstinner vstinner deleted the WIP_type_fini branch January 21, 2022 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 approved these changes

@shihai1991 shihai1991 shihai1991 approved these changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vstinner @shihai1991 @corona10 @the-knights-who-say-ni @bedevere-bot