GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-94673: Ensure Builtin Static Types are Readied Properly by ericsnowcurrently · Pull Request #103940 · python/cpython

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Apr 27, 2023 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There were cases where we do unnecessary work for builtin static types. This also simplifies some work necessary for a per-interpreter GIL.

  • Issue: [subinterpreters] Static types incorrectly share some objects between interpreters #94673

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ericsnowcurrently
Use _PyType_IsReady().
721e7ab
@ericsnowcurrently ericsnowcurrently added the skip news label Apr 27, 2023
@ericsnowcurrently
ericsnowcurrently requested review from iritkatriel and markshannon as code owners April 27, 2023 18:58
@bedevere-bot bedevere-bot mentioned this pull request Apr 27, 2023
[subinterpreters] Static types incorrectly share some objects between interpreters #94673
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Apr 27, 2023
@ericsnowcurrently
ericsnowcurrently removed request for iritkatriel and markshannon April 27, 2023 18:58
ericsnowcurrently added 4 commits April 27, 2023 14:50
@ericsnowcurrently
Make sure _PyStaticType_InitBuiltin() is called for all interpreters.
0792ff3
@ericsnowcurrently
Do not call PyType_Ready() unnecessarily.
447f89a
@ericsnowcurrently
Do not call PyType_Ready() in _PyStructSequence_InitBuiltinWithFlags().
b3bd674
@ericsnowcurrently
Do more in type_ready().
143f936
@ericsnowcurrently
ericsnowcurrently force-pushed the ensure-builtin-static-type-ready branch from 674427b to 143f936 Compare April 27, 2023 20:50
@ericsnowcurrently
Be slightly less strict in PyType_Ready().
4a4109e
@ericsnowcurrently
ericsnowcurrently merged commit d2e2e53 into python:main Apr 27, 2023
@ericsnowcurrently
ericsnowcurrently deleted the ensure-builtin-static-type-ready branch April 27, 2023 22:19
@bedevere-bot bedevere-bot removed the awaiting core review label Apr 27, 2023
@ericsnowcurrently ericsnowcurrently mentioned this pull request Apr 27, 2023
gh-94673: Fix _PyTypes_InitTypes() and get_type_attr_as_size() #103961
Merged
ericsnowcurrently added a commit that referenced this pull request Apr 28, 2023
@ericsnowcurrently
gh-94673: Fix _PyTypes_InitTypes() and get_type_attr_as_size() (gh-10…
424a785
…3961)

This change has two small parts:

1. a follow-up to gh-103940 with one case I missed
2. adding a missing return that I noticed while working on related code
@Forty-Bot

Forty-Bot commented Nov 19, 2023

Copy link
Copy Markdown

This causes a regression for some users of PyModule_AddType. Previously, you could do something like

PyTypeObject MyType = {
	PyObject_HEAD_INIT(NULL)
	/* ... */
};

int MyType_Add(PyObject *m)
{
	int err;
	PyObject *obj = PyLong_FromDouble(1.0);

	if (!obj)
		return -1;

	MyType.tp_dict = PyDict_New();
	if (!type->tp_dict) {
		Py_DECREF(obj);
		return -1;
	}

	err = PyDict_SetItemString(MyType.tp_dict, "foo", obj);
	Py_DECREF(obj);
	if (err)
		return err;

	if (PyModule_AddType(m, &MyType))
		return -1;

	return 0;
}

but now this will now segfault, as PyModule_AddType will never call PyType_Ready since tp_dict is initialized. The documentation for PyModule_AddType says:

Add a type object to module. The type object is finalized by calling internally PyType_Ready().

with no mention of tp_dict. On the subject of when to call PyType_Ready, the documentation for PyObject.ob_type says:

Therefore, the convention is to pass NULL to the PyObject_HEAD_INIT macro and to initialize this field explicitly at the start of the module’s initialization function, before doing anything else...

This should be done before any instances of the type are created.

PyTypeObject.tp_dict may also be initialized before calling PyType_Ready:

This field should normally be initialized to NULL before PyType_Ready is called; it may also be initialized to a dictionary containing initial attributes for the type.

As PyModule_AddType is part of the stable ABI, I believe that using _PyType_IsReady in PyModule_AddType should be reverted.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Forty-Bot added a commit to Forty-Bot/mpmetrics that referenced this pull request Nov 19, 2023
@Forty-Bot
Fix c extension on python 3.12
8f76168
Workaround for [1].

[1] python/cpython#103940

Signed-off-by: Sean Anderson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@ericsnowcurrently @Forty-Bot @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.