Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-124470: Fix crash when reading from object instance dictionary while replacing it#122489

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
DinoV merged 4 commits intopython:mainfromDinoV:nogil_dict_incref
Nov 21, 2024

Conversation

@DinoV
Copy link
Contributor

@DinoVDinoV commentedJul 31, 2024•
edited
Loading

Currently when we have one thread reading an attribute from an object and another thread replacing the dictionary we can crash. This is because the reader thread is actually using a borrowed reference to the object and the writer will decref the dictionary and de-allocate it when replacing the dictionary.

This fixes this by changing the decref of the previous dictionary to be delayed via QSBR. We get rid of theGC_SET_SHARED_INLINE flag which is not being used anywhere and instead we simply queue the decref via_PyObject_XDecRefDelayed.

When we process these objects during GC we need to be careful because we don't want to run object finalizers during the destruction of the objects. Instead we do the same thing we do w/ merged dec refs and queue the objects to be decref'd once the world has resumed.

@DinoVDinoVforce-pushed thenogil_dict_incref branch 3 times, most recently fromc26d357 to5a1b6fdCompareJuly 31, 2024 17:50
@DinoVDinoVforce-pushed thenogil_dict_incref branch 4 times, most recently from16ca39f to0988d1aCompareAugust 13, 2024 21:50
@DinoVDinoVforce-pushed thenogil_dict_incref branch 2 times, most recently from0169a30 tobd1c1a7CompareSeptember 24, 2024 20:44
@DinoVDinoV changed the titleIncref dict when it's usage is borrowedgh-124470: Incref dict when it's usage is borrowedSep 24, 2024
@DinoVDinoVforce-pushed thenogil_dict_incref branch 4 times, most recently from0146a13 to2d05896CompareSeptember 25, 2024 22:58
@DinoVDinoV marked this pull request as ready for reviewSeptember 25, 2024 23:33
Copy link
Contributor

@colesburycolesbury left a comment

Choose a reason for hiding this comment

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

The overall approach makes sense to me. A few comments below.

}

FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
(PyDictObject*)Py_XNewRef(new_dict));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: align(PyDictObject *)Py_XNewRef(new_dict)

DinoV reacted with thumbs up emoji
@DinoVDinoVforce-pushed thenogil_dict_incref branch 4 times, most recently from1a199bf todc3d5aeCompareSeptember 26, 2024 23:16
@DinoVDinoV changed the titlegh-124470: Incref dict when it's usage is borrowedgh-124470: Fix crash when reading from object instance dictionary while replacing itSep 27, 2024
Copy link
Contributor

@colesburycolesbury left a comment

Choose a reason for hiding this comment

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

LGTM. A few minor comments below

// zero. Otherwise, the thread gives up ownership and merges the reference
// count fields.
PyAPI_FUNC(void)_Py_MergeZeroLocalRefcount(PyObject*);

Copy link
Contributor

Choose a reason for hiding this comment

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

Stray whitespace change

DinoV reacted with thumbs up emoji
set_or_clear_managed_dict(PyObject*obj,PyObject*new_dict,boolclear)
{
assert(Py_TYPE(obj)->tp_flags&Py_TPFLAGS_MANAGED_DICT);
assert(_PyObject_InlineValuesConsistencyCheck(obj));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these consistency checks need to be within some sort of lock?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

The type flag one is fine, I'll add some ifdefs and lock for the inline check.

Comment on lines 7149 to 7113
FT_ATOMIC_STORE_PTR(_PyObject_ManagedDictPointer(obj)->dict,
(PyDictObject*)Py_XNewRef(new_dict));
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe align the wrapped line

DinoV reacted with thumbs up emoji
@colesburycolesbury added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelSep 30, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@colesbury for commitdc3d5ae 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelSep 30, 2024
@DinoVDinoVforce-pushed thenogil_dict_incref branch 2 times, most recently from117a19e to367e1f4CompareNovember 20, 2024 21:57
@DinoVDinoV added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelNov 21, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@DinoV for commitc9aee55 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelNov 21, 2024
@DinoVDinoV merged commitbf542f8 intopython:mainNov 21, 2024
54 of 55 checks passed
ebonnal pushed a commit to ebonnal/cpython that referenced this pull requestJan 12, 2025
…ry while replacing it (python#122489)Delay free a dictionary when replacing it
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@colesburycolesburycolesbury approved these changes

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

@methanemethaneAwaiting requested review from methanemethane is a code owner

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DinoV@bedevere-bot@colesbury
[8]ページ先頭
©2009-2025 Movatter.jp