GeezerVison

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

GeezerVison is showing github.com — gh-112075: Fix race in constructing dict for instance by DinoV · Pull Request #118499 · python/cpython

Conversation

DinoV commented May 2, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Fix a small thread safety issue w/ dicts. When we are constructing the dict for an object with either a non-inline managed dict or with a non-managed dict we're not properly protecting the creation with a lock. This locks the object in _PyObjectDict_SetItem if we don't already have a dict to ensure only one thread creates the lock at a time.

Issue: Make dict objects thread-safe in --disable-gil builds #112075

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-app Bot mentioned this pull request

May 2, 2024

Make dict objects thread-safe in --disable-gil builds #112075

Closed

DinoV added skip news topic-free-threading labels

May 2, 2024

DinoV requested a review from colesbury

May 2, 2024 02:46

DinoV force-pushed the nogil_dict_creation branch from 12d0685 to ae6d925 Compare

May 2, 2024 02:48

DinoV marked this pull request as ready for review

May 2, 2024 05:29

DinoV requested review from markshannon and methane as code owners

May 2, 2024 05:29

bedevere-app Bot added the awaiting core review label

May 2, 2024

colesbury reviewed

May 2, 2024

View reviewed changes

colesbury left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We also need to fix:

PyObject_GenericGetDict (non-managed dict case)
PyObject_GenericSetDict

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/dictobject.c Outdated

		assert(dictptr != NULL);
		if ((tp->tp_flags & Py_TPFLAGS_HEAPTYPE) && (cached = CACHED_KEYS(tp))) {
		assert(dictptr != NULL);
		dict = *dictptr;

colesbury May 2, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We need to use atomics to load from the dictptr or there will be a data race between the read here and the write within the critical section.

Sorry, something went wrong.

Uh oh!

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

DinoV reacted with thumbs up emoji

All reactions

👍 1 reaction

DinoV force-pushed the nogil_dict_creation branch 2 times, most recently from a9ad363 to 56a79e6 Compare

May 3, 2024 19:50

colesbury self-requested a review

May 3, 2024 20:01

colesbury reviewed

May 3, 2024

View reviewed changes

Comment thread

Objects/dictobject.c Outdated


		Py_BEGIN_CRITICAL_SECTION(dict);
		res = set_or_del_lock_held((PyDictObject *)dict, key, value);
		Py_END_CRITICAL_SECTION();

colesbury May 3, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think the ASSERT_CONSISTENT needs to be inside the critical section.

Sorry, something went wrong.

Uh oh!

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

DinoV reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Objects/dictobject.c Outdated

		{
		PyDictKeysObject *cached;

		PyObject dict = FT_ATOMIC_LOAD_PTR_RELAXED(dictptr);

colesbury May 3, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

acquire (or seq-cst)

Sorry, something went wrong.

Uh oh!

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

DinoV reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Objects/dictobject.c Outdated

		else {
		dict = PyDict_New();
		}
		FT_ATOMIC_STORE_PTR_RELAXED(*dictptr, dict);

colesbury May 3, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

release (or seq-cst)

Sorry, something went wrong.

Uh oh!

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

DinoV reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Objects/dictobject.c Outdated

Comment on lines +7217 to +7219

		if (dict == NULL) {
		dictkeys_decref(interp, cached, false);
		}

colesbury May 3, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think new_dict_with_shared_keys already handles the dictkeys_decref in the error case.

Sorry, something went wrong.

Uh oh!

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

DinoV reacted with thumbs up emoji

All reactions

👍 1 reaction

DinoV May 6, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oh, that's super subtle, I don't think there's any reason to spread this out, I'll move the incref into new_dict_with_shared_keys

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Objects/object.c Outdated

		return -1;
		}
		#ifdef Py_GIL_DISABLED
		Py_XDECREF(_Py_atomic_exchange_ptr(dictptr, Py_NewRef(value)));

colesbury May 3, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think we need to be consistent about either using atomic exchange/cas or using the object locks to protect the dictptr. I don't think we can safely mix them.

Sorry, something went wrong.

Uh oh!

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

DinoV reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Objects/dictobject.c Outdated

		}
		#endif
		PyTypeObject *tp = Py_TYPE(obj);
		if ((tp->tp_flags & Py_TPFLAGS_HEAPTYPE) && (cached = CACHED_KEYS(tp))) {

colesbury May 3, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

maybe: _PyType_HasFeature(tp, Py_TPFLAGS_HEAPTYPE)

Sorry, something went wrong.

Uh oh!

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

DinoV reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Objects/dictobject.c Outdated

		goto done;
		}
		#endif
		OBJECT_STAT_INC(dict_materialized_on_request);

colesbury May 3, 2024

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Are we consistent about when we increment this stat?

Sorry, something went wrong.

Uh oh!

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

All reactions

DinoV May 6, 2024

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We seem to actually have an extra call in materialize_managed_dict_lock_held which is the inline values case. This one seems like maybe it shouldn't exist at all, as we no longer have a simple values array which is outside of the dict, so even though we have shared keys here we will always have a dict. I'll remove it from here.

Sorry, something went wrong.

Uh oh!

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

All reactions

DinoV force-pushed the nogil_dict_creation branch from e7e5ed4 to f328d37 Compare

May 6, 2024 18:40

colesbury approved these changes

May 6, 2024

View reviewed changes

colesbury left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. Two minor comments below

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_free_threading/test_dict.py Resolved

Uh oh!

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

Comment thread

Objects/dictobject.c Resolved

Uh oh!

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

bedevere-app Bot added awaiting merge and removed awaiting core review labels

May 6, 2024

DinoV added 4 commits

May 6, 2024 14:55


          Fix race in constructing dict for instance

05092ed


          Fix PyObject_GenericGetDict, PyObject_GenericSetDict, and add test cases

2db4a3c


          Simplify incref's around new_dict_with_shared_keys, fix stats

f3f3445

Lock obj instead of using atomic, and fix reads/writes


          remove stale comment and use requires_working_threading

9547a5f

DinoV force-pushed the nogil_dict_creation branch from f328d37 to 9547a5f Compare

May 6, 2024 22:01

ambv enabled auto-merge (squash)

May 6, 2024 23:18

ambv merged commit 636b8d9 into python:main

May 6, 2024

bedevere-app Bot removed the awaiting merge label

May 6, 2024

SonicField pushed a commit to SonicField/cpython that referenced this pull request

May 8, 2024


          pythongh-112075: Fix race in constructing dict for instance (python#1…

4315b41

…18499)

DinoV deleted the nogil_dict_creation branch

May 31, 2024 18:21

Reviewers

colesbury

colesbury approved these changes

methane

Awaiting requested review from methane

methane is a code owner

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

skip news topic-free-threading

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

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 01402520 — 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.

Conversation

DinoV commented May 2, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DinoV commented May 2, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

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