8000 [3.4] bpo-26617: Ensure gc tracking is off when invoking weakref callbacks. by serhiy-storchaka · Pull Request #2695 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

[3.4] bpo-26617: Ensure gc tracking is off when invoking weakref callbacks. #2695

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 22, 2017
Merged

[3.4] bpo-26617: Ensure gc tracking is off when invoking weakref callbacks. #2695

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad0aab6
[3.4] bpo-26617: Ensure gc tracking is off when invoking weakref call…
benjaminp Oct 4, 2016
0ce5c15
Rewrite a NEWS entry as a NEWS.d entry.
serhiy-storchaka Jul 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[3.4] bpo-26617: Ensure gc tracking is off when invoking weakref call… 
…backs.

(cherry picked from commit 8f657c3)
  • Loading branch information
@benjaminp @serhiy-storchaka
benjaminp authored and serhiy-storchaka committed Jul 13, 2017
commit ad0aab6e27e85e99a8fb28b7aaf9523e4c0b7388
8 changes: 8 additions & 0 deletions Lib/test/test_weakref.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,14 @@ def test_set_callback_attribute(self):
with self.assertRaises(AttributeError):
ref1.__callback__ = lambda ref: None

def test_callback_gcs(self):
class ObjectWithDel(Object):
def __del__(self): pass
x = ObjectWithDel(1)
ref1 = weakref.ref(x, lambda ref: support.gc_collect())
del x
support.gc_collect()


class SubclassableWeakrefTestCase(TestBase):

Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ Release date: XXXX-XX-XX
Core and Builtins
-----------------

- Issue #26617: Fix crash when GC runs during weakref callbacks.
Copy link
Member

Choose a reason for hiding this comment

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

You may rewrite this as a NEWS.d entry with blurb, to prevent conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, please do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to use blurb, entered all data, but it doesn't have any effect.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you ran blurb and entered data, and it didn't print an error, then it had an effect. It creates a file in Misc/NEWS.d/next and runs "git add" on that file.

Were you inside this repo when you ran it?

If it's more convenient, you could copy an existing file in Misc/NEWS.d/next and manually change the filename to match the current time. But we should figure out why blurb isn't working for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, this is my fault! Sorry. I executed git diff and git log, but their didn't show any changes. I needed to use git diff --staged or git status instead.


- bpo-27945: Fixed various segfaults with dict when input collections are
mutated during searching, inserting or comparing. Based on patches by
Duane Griffin and Tim Mitchell.
Expand Down
25 changes: 13 additions & 12 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1116,11 +1116,6 @@ subtype_dealloc(PyObject *self)
Py_TRASHCAN_SAFE_BEGIN(self);
--_PyTrash_delete_nesting;
-- tstate->trash_delete_nesting;
/* DO NOT restore GC tracking at this point. weakref callbacks
* (if any, and whether directly here or indirectly in something we
* call) may trigger GC, and if self is tracked at that point, it
* will look like trash to GC and GC will try to delete self again.
*/

/* Find the nearest base with a different tp_dealloc */
base = type;
Expand All @@ -1131,30 +1126,36 @@ subtype_dealloc(PyObject *self)

has_finalizer = type->tp_finalize || type->tp_del;

/* Maybe call finalizer; exit early if resurrected */
if (has_finalizer)
_PyObject_GC_TRACK(self);

if (type->tp_finalize) {
_PyObject_GC_TRACK(self);
if (PyObject_CallFinalizerFromDealloc(self) < 0) {
/* Resurrected */
goto endlabel;
}
_PyObject_GC_UNTRACK(self);
}
/* If we added a weaklist, we clear it. Do this *before* calling
tp_del, clearing slots, or clearing the instance dict. */
/*
If we added a weaklist, we clear it. Do this *before* calling tp_del,
clearing slots, or clearing the instance dict.

GC tracking must be off at this point. weakref callbacks (if any, and
whether directly here or indirectly in something we call) may trigger GC,
and if self is tracked at that point, it will look like trash to GC and GC
will try to delete self again.
*/
if (type->tp_weaklistoffset && !base->tp_weaklistoffset)
PyObject_ClearWeakRefs(self);

if (type->tp_del) {
_PyObject_GC_TRACK(self);
type->tp_del(self);
if (self->ob_refcnt > 0) {
/* Resurrected */
goto endlabel;
}
_PyObject_GC_UNTRACK(self);
}
if (has_finalizer) {
_PyObject_GC_UNTRACK(self);
/* New weakrefs could be created during the finalizer call.
If this occurs, clear them out without calling their
finalizers since they might rely on part of the object
Expand Down
0