gh-95348: Split clear_weakref into two functions #95449

PurityLake · 2022-07-29T19:29:49Z

Moves the removal of the passed in reference from a list to a function called remove_weakref adding a parameter called list that is a list of weak references. Then to abstract the need to get this list a function remove_weakref_from_referent has been added to automatically pass the list to the remove_weakref.

This is added to make the process more transparent as to what it is doing because as the issue mentions that where clear_weakref is used it isn't clear what it is doing.

Functions added:

remove_weakref
remove_weakref_from_referent

remove_weakref shouldn't be called by itself as remove_weakref_from_referent will call it and pass the list of references.

Also remove_weakref_from_referent should always be called after `clear_weakref, otherwise a segmentation fault will occur.

3 tests were found to have failed but seem to occur even without changes to code:

test_strftime
test_strptime
test_time

Issue: clear_weakref() in Objects/weakrefobject.c is Confusing #95348

ghost · 2022-07-29T19:29:51Z

All commit authors signed the Contributor License Agreement.

bedevere-bot · 2022-07-29T19:29:53Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

Objects/weakrefobject.c

ericsnowcurrently

I've left some comments.

Most importantly, the weakref should be removed from the weaklist before clear_weakref() is called, not after.

ericsnowcurrently · 2022-08-01T18:12:53Z

Objects/weakrefobject.c

-               then the weakref list itself (and thus the value of *list) will
-               end up being set to NULL. */
-            *list = self->wr_next;
-        self->wr_object = Py_None;


Clearing wr_object should still happen in clear_weakref(), not remove_weakref().

The thing is, the program segfaults when I reverse the order of remove before clear

weaklist_remove_weakref(list, *list); clear_weakref(*list);

causes a segfault for me.

ericsnowcurrently · 2022-08-01T18:14:05Z

Objects/weakrefobject.c

    if (callback != NULL) {
        Py_DECREF(callback);
        self->wr_callback = NULL;
    }
 }

+/* This function removes the pass in reference from the list of weak


Suggested change

/* This function removes the pass in reference from the list of weak

/* This function removes the passed-in reference from the list of weak

ericsnowcurrently · 2022-08-01T18:17:12Z

Objects/weakrefobject.c

+/* This function is responsible for clearing the callback slot. When using this
+ * function it must be called before calling remove_weakref_from_referent,
+ * otherwise a segmentation fault will occur at build time.


The function should be clearing the weakref object. So:

Suggested change

/* This function is responsible for clearing the callback slot. When using this

* function it must be called before calling remove_weakref_from_referent,

* otherwise a segmentation fault will occur at build time.

/* This function clears the passed-in reference. It must have been removed

* from the weaklist already. (See weaklist_remove_weakref().)

ericsnowcurrently · 2022-08-01T18:19:12Z

Objects/weakrefobject.c

+ * references for the referent. This should be called from 
+ * remove_weakref_fromt_referent as it will pass the required list
+ * argument.


There is no need for that last sentence.

Suggested change

* references for the referent. This should be called from

* remove_weakref_fromt_referent as it will pass the required list

* argument.

* references for the referent.

ericsnowcurrently · 2022-08-01T18:23:15Z

Objects/weakrefobject.c

+    self->wr_next = NULL;
+}
+
+/* This function removes the pass in reference from the list of weak references 


Suggested change

/* This function removes the pass in reference from the list of weak references

/* This function removes the passed-in reference from the list of weak references

ericsnowcurrently · 2022-08-01T18:26:54Z

Objects/weakrefobject.c

+            end up being set to NULL. */
+        *list = self->wr_next;
+    }
+    self->wr_object = Py_None;


This belongs in clear_weakref().

Suggested change

self->wr_object = Py_None;

ericsnowcurrently · 2022-08-01T18:33:16Z

Misc/NEWS.d/next/C API/2022-07-29-19-35-30.gh-issue-95348.p5CDaH.rst

+Split ``clear_weakref`` into two functions: ``clear_weakref``
+and ``remove_weakref``. Abstracts ``remove_weakref`` with
+``remove_weakref_from_referent`` in weakrefobject.c.
+Patch by Robert O'Shea.


I'm not sure that a NEWS entry is warranted. If this PR changes any behavior then something has gone wrong, so there isn't anything to communicate to users. This change is strictly about slightly improving the clarity of the Objects/weakrefobject.c code. That sort of minor maintainability improvement typically does not get a NEWS entry.

ericsnowcurrently · 2022-08-01T18:38:52Z

Objects/weakrefobject.c

+ * argument.
+ */
+static void
+remove_weakref(PyWeakReference *self, PyWeakReference **list)


This function is an operation on a weaklist, rather than on a single weakref, so let's change the name and signature accordingly:

Suggested change

remove_weakref(PyWeakReference *self, PyWeakReference **list)

weaklist_remove_ref(PyWeakReference **list, PyWeakReference *ref)

ericsnowcurrently · 2022-08-01T18:41:28Z

Objects/weakrefobject.c

+ * for the referent. This should be called after clear_weakref, otherwise a
+ * segmentation fault will occur at build time.


We don't need to mention segfaulting.

Suggested change

* for the referent. This should be called after clear_weakref, otherwise a

* segmentation fault will occur at build time.

* for the referent. This should always be called before clear_weakref().

bedevere-bot · 2022-08-01T18:44:57Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ericsnowcurrently · 2022-08-01T18:48:23Z

Objects/weakrefobject.c

@@ -958,8 +983,11 @@ PyObject_ClearWeakRefs(PyObject *object)
    /* Remove the callback-less basic and proxy references */
    if (*list != NULL && (*list)->wr_callback == NULL) {
        clear_weakref(*list);
-        if (*list != NULL && (*list)->wr_callback == NULL)
+        remove_weakref_from_referent(*list);


Here we already have the list, so we should use weaklist_remove_weakref() directly:

Suggested change

remove_weakref_from_referent(*list);

weaklist_remove_weakref(list, *list);

ericsnowcurrently · 2022-08-01T18:48:39Z

Objects/weakrefobject.c

            clear_weakref(*list);
+            remove_weakref_from_referent(*list);


Likewise with weaklist_remove_weakref().

ericsnowcurrently · 2022-08-01T18:48:47Z

Objects/weakrefobject.c

@@ -972,6 +1000,7 @@ PyObject_ClearWeakRefs(PyObject *object)

            current->wr_callback = NULL;
            clear_weakref(current);
+            remove_weakref_from_referent(current);


Likewise with weaklist_remove_weakref().

ericsnowcurrently · 2022-08-01T18:49:06Z

Objects/weakrefobject.c

@@ -1002,6 +1031,7 @@ PyObject_ClearWeakRefs(PyObject *object)
                }
                current->wr_callback = NULL;
                clear_weakref(current);
+                remove_weakref_from_referent(current);


Likewise with weaklist_remove_weakref().

ericsnowcurrently · 2022-08-01T18:49:29Z

Objects/weakrefobject.c

@@ -1036,5 +1066,6 @@ _PyStaticType_ClearWeakRefs(PyTypeObject *type)
           weaklist before clearing its wr_object and wr_callback.
           That is how we're able to loop over the list. */
        clear_weakref((PyWeakReference *)*list);
+        remove_weakref_from_referent((PyWeakReference *)*list);


Likewise with weaklist_remove_weakref().

PurityLake · 2022-08-05T12:41:48Z

@ericsnowcurrently

The issue with calling clear first is that wr_object is set to Py_None. Making it impossible to get the list after the fact.

ericsnowcurrently · 2022-08-05T15:48:58Z

(This advice applies to everyone, not just you. 😄)

FYI, there are downsides to force-pushing once other people have started interacting with a PR:

it causes the reviewed commits to go away, making it harder to re-review
if you make edits to earlier commits, It makes it much harder to see what changes were made in response to a review
earlier CI runs mostly become inaccessible
it's a slight inconvenience for anyone that pulls down your branch

So I recommend that, as soon as others are interacting with your PR (e.g. reviewing), you should avoid force-pushing. If you need to update the base branch (e.g. main) then use "merge" instead.

All that said, force-pushing is usually fine up to the point that other people start looking at the PR. Personally, I use force-push extensively, but try to stick to the above advice (once others start participating).

ericsnowcurrently · 2022-08-05T15:54:51Z

The issue with calling clear first is that wr_object is set to Py_None. Making it impossible to get the list after the fact.

That only matters if you use remove_weakref_from_referent(). Instead, use weaklist_remove_weakref().

PurityLake · 2022-08-05T18:00:27Z

I have made the requested changes; please review again

This should be correct now.

Apologies for the issue with the force push.

bedevere-bot · 2022-08-05T18:00:31Z

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

bedevere-bot added the awaiting review label Jul 29, 2022

corona10 requested a review from ericsnowcurrently July 30, 2022 10:05

erlend-aasland reviewed Aug 1, 2022

View reviewed changes

Objects/weakrefobject.c Outdated Show resolved Hide resolved

Objects/weakrefobject.c Outdated Show resolved Hide resolved

PurityLake force-pushed the fix-issue-95348 branch from 2e5d65d to 037ee37 Compare August 1, 2022 17:41

ericsnowcurrently requested changes Aug 1, 2022

View reviewed changes

bedevere-bot removed the awaiting review label Aug 1, 2022

bedevere-bot added the awaiting changes label Aug 1, 2022

ericsnowcurrently requested changes Aug 1, 2022

View reviewed changes

PurityLake and others added 4 commits August 5, 2022 15:03

pythongh-95348: Split clear_weakref into two functions

06c9d7e

📜🤖 Added by blurb_it.

40f247f

pythongh-95348 Update NEWS blub

8a24c4f

pythongh-95348: Update new functions in weakrefobject.c

355625a

PurityLake force-pushed the fix-issue-95348 branch from 037ee37 to 355625a Compare August 5, 2022 14:04

PurityLake and others added 2 commits August 5, 2022 18:06

Merge branch 'python:main' into fix-issue-95348

8ec2929

pythongh-95348: Update weakrefobject to use new function

1e8e522

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 5, 2022

bedevere-bot requested a review from ericsnowcurrently August 5, 2022 18:00

PurityLake closed this Dec 25, 2022

PurityLake deleted the fix-issue-95348 branch December 25, 2022 02:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-95348: Split clear_weakref into two functions #95449

gh-95348: Split clear_weakref into two functions #95449

PurityLake commented Jul 29, 2022 •

edited

Loading

ghost commented Jul 29, 2022 •

edited by ghost

Loading

bedevere-bot commented Jul 29, 2022

ericsnowcurrently left a comment

ericsnowcurrently Aug 1, 2022

PurityLake Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

bedevere-bot commented Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

ericsnowcurrently Aug 1, 2022

PurityLake commented Aug 5, 2022

ericsnowcurrently commented Aug 5, 2022

ericsnowcurrently commented Aug 5, 2022

PurityLake commented Aug 5, 2022

bedevere-bot commented Aug 5, 2022

	/* This function removes the pass in reference from the list of weak
	/* This function removes the passed-in reference from the list of weak

	remove_weakref(PyWeakReference self, PyWeakReference *list)
	weaklist_remove_ref(PyWeakReference *list, PyWeakReference ref)

		* for the referent. This should be called after clear_weakref, otherwise a
		* segmentation fault will occur at build time.

	* for the referent. This should be called after clear_weakref, otherwise a
	* segmentation fault will occur at build time.
	* for the referent. This should always be called before clear_weakref().

	remove_weakref_from_referent(*list);
	weaklist_remove_weakref(list, *list);

gh-95348: Split clear_weakref into two functions #95449

gh-95348: Split clear_weakref into two functions #95449

Conversation

PurityLake commented Jul 29, 2022 • edited Loading

ghost commented Jul 29, 2022 • edited by ghost Loading

bedevere-bot commented Jul 29, 2022

ericsnowcurrently left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PurityLake commented Aug 5, 2022

ericsnowcurrently commented Aug 5, 2022

ericsnowcurrently commented Aug 5, 2022

PurityLake commented Aug 5, 2022

bedevere-bot commented Aug 5, 2022

PurityLake commented Jul 29, 2022 •

edited

Loading

ghost commented Jul 29, 2022 •

edited by ghost

Loading