GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-154527: Fix data-race in `defaultdict_repr` and `defaultdict_missing` by brijkapadia · Pull Request #154560 · python/cpython Conversation
|
| static PyObject * | ||
| defdict_repr(PyObject *op) | ||
| { | ||
| PyObject *return_value; | ||
| Py_BEGIN_CRITICAL_SECTION(op); | ||
| return_value = defdict_repr_lock_held(op); | ||
| Py_END_CRITICAL_SECTION(); | ||
| return return_value; | ||
| } | ||
|
|
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.
Hey @brijkapadia I stumbled across this PR while looking at free-threading issues and thought I’d share a slightly different approach.
The current change (clinic @critical_section on __missing__ + wrapping the whole of defdict_repr) holds the per-object lock across the call to the factory and across PyObject_Repr.
That fixes the data race but it also means we’re executing arbitrary Python while still holding the lock so if the factory (or a custom __repr__) ends up touching the same defaultdict again or tries to take other locks then things can get uncomfortable.
The pattern that seems preferred in the other free-threading cleanups is to take a strong reference under the lock and then drop it immediately. For the repr path that would look like:
| static PyObject * | |
| defdict_repr(PyObject *op) | |
| { | |
| PyObject *return_value; | |
| Py_BEGIN_CRITICAL_SECTION(op); | |
| return_value = defdict_repr_lock_held(op); | |
| Py_END_CRITICAL_SECTION(); | |
| return return_value; | |
| } | |
| static PyObject * | |
| defdict_repr(PyObject *op) | |
| { | |
| defdictobject *dd = defdictobject_CAST(op); | |
| PyObject *baserepr; | |
| PyObject *defrepr; | |
| PyObject *factory; | |
| PyObject *result; | |
| baserepr = PyDict_Type.tp_repr(op); | |
| if (baserepr == NULL) | |
| return NULL; | |
| Py_BEGIN_CRITICAL_SECTION(dd); | |
| factory = Py_XNewRef(dd->default_factory); | |
| Py_END_CRITICAL_SECTION(); | |
| if (factory == NULL) { | |
| defrepr = PyUnicode_FromString("None"); | |
| } | |
| else { | |
| int status = Py_ReprEnter(factory); | |
| if (status != 0) { | |
| if (status < 0) { | |
| Py_DECREF(baserepr); | |
| Py_DECREF(factory); | |
| return NULL; | |
| } | |
| defrepr = PyUnicode_FromString("..."); | |
| } | |
| else { | |
| defrepr = PyObject_Repr(factory); | |
| Py_ReprLeave(factory); | |
| } | |
| Py_DECREF(factory); | |
| } | |
| if (defrepr == NULL) { | |
| Py_DECREF(baserepr); | |
| return NULL; | |
| } | |
| result = PyUnicode_FromFormat("%s(%U, %U)", | |
| _PyType_Name(Py_TYPE(dd)), | |
| defrepr, baserepr); | |
| Py_DECREF(defrepr); | |
| Py_DECREF(baserepr); | |
| return result; | |
| } |
That way the lock is only held for the tiny window needed to take the reference and everything after that is safe because we own the object.
Good catch btw and have a great day!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
This PR is stale because it has been open for 90 days with no activity. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
1 more reviewer
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.