Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-40895: Update weakref documentation to remove old warnings #20687

Merged

Conversation

asqui
Copy link
Contributor

@asqui asqui commented Jun 6, 2020

The doccumentation at https://docs.python.org/3.10/library/weakref.html cautions that the WeakKeyDictionary and WeakValueDictionary are susceptible to the problem of dictionary mutation during iteration.

These notes present the user with a problem that has no easy solution.

I dug into the implementation and found that fortunately, Antoine Pitrou already addressed this challenge (10 years ago!) by introducing an _IterationGuard context manager to the implementation, which delays mutation while an iteration is in progress.

I asked for confirmation and @pitrou agreed that these notes could be removed:
c1baa60#commitcomment-39514438

https://bugs.python.org/issue40895

Automerge-Triggered-By: @pitrou

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Jun 6, 2020
asqui referenced this pull request Jun 6, 2020
…ainst

the destruction of weakref'ed objects while iterating.
@njsmith njsmith changed the title bpo-31254: Update weakref documentation to remove old warnings bpo-7105: Update weakref documentation to remove old warnings Jun 6, 2020
@njsmith njsmith changed the title bpo-7105: Update weakref documentation to remove old warnings bpo-40895: Update weakref documentation to remove old warnings Jun 6, 2020
@asqui asqui force-pushed the remove-weakref-dict-warnings-bpo-31254 branch from 0443f9e to d566ae9 Compare June 7, 2020 00:00
@asqui
Copy link
Contributor Author

asqui commented Jun 7, 2020

Slight copy-paste failure on my part resulting in me using the wrong BPO issue number throughout.

The title and body of this PR have been corrected and I've force-pushed a corrected commit message. Please excuse the wrong BPO issue in the underlying branch name -- it's not possible to fix that without closing the PR and opening a new one.

@aeros aeros added the skip news label Jun 7, 2020
@aeros
Copy link
Contributor

aeros commented Jun 7, 2020

Please excuse the wrong BPO issue in the underlying branch name -- it's not possible to fix that without closing the PR and opening a new one.

That's fine, I've certainly made that mistake myself. While the name makes it easier to identify the purpose of branch when reviewing multiple ongoing PRs at the same time, it's definitely not a significant issue. The more important part is the PR title for discoverability and proper linking to the bpo issue, but that's been fixed. So, I wouldn't worry about it. :-)

Copy link
Contributor

@aeros aeros left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @asqui and for bringing this to our attention. It's often easy to forget to update old parts of the documentation, so we greatly appreciate the reminder; even more so when a PR is opened to address it.

LGTM. From looking over the other parts of the documentation, I don't see any other warnings or notes about not changing the size of the WeakDictionarys during iteration that should be removed. I'll leave it to @pitrou to give a final review though since he made the underlying code changes.

@aeros aeros added needs backport to 3.8 only security fixes needs backport to 3.9 only security fixes labels Jun 7, 2020
@aeros
Copy link
Contributor

aeros commented Jun 7, 2020

We might be able to backport this to earlier versions considering the age of the commit, but we typically backport documentation changes only down to the current default version for docs.python.org (3.8 at the moment), as that gives the most significant benefit to the highest number of users.

Antoine, do you think this case might be an exception or should we just backport to 3.9 and 3.8? The removal of this warning seems quite significant and can expand the user perception for usage of the weakref dictionaries, so it might be relevant to users looking at older versions of the documentation. I'm thinking we could pretty reasonably backport it to 3.7 as well, but we should probably double check w/ Ned first since that branch is just a couple versions away from being security-only.

@aeros aeros requested a review from pitrou June 7, 2020 10:33
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

+1, LGTM

@pitrou
Copy link
Member

pitrou commented Jun 10, 2020

Given that we're fixing an oversight in a previous commit, I think it's fine to backport to 3.7.

@miss-islington miss-islington merged commit 1642c0e into python:master Jun 10, 2020
@miss-islington
Copy link
Contributor

Thanks @asqui for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8, 3.9.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-20791 is a backport of this pull request to the 3.9 branch.

@miss-islington
Copy link
Contributor

Sorry, @asqui, I could not cleanly backport this to 3.8 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 1642c0ef750f96664a98cadb09301d492098d2fb 3.8

@miss-islington miss-islington self-assigned this Jun 10, 2020
@miss-islington
Copy link
Contributor

Sorry @asqui, I had trouble checking out the 3.7 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 1642c0ef750f96664a98cadb09301d492098d2fb 3.7

pitrou pushed a commit to pitrou/cpython that referenced this pull request Jun 10, 2020
…ythonGH-20687)

The doccumentation at https://docs.python.org/3.10/library/weakref.html cautions that the `WeakKeyDictionary` and `WeakValueDictionary` are susceptible to the problem of dictionary mutation during iteration.

These notes present the user with a problem that has no easy solution.

I dug into the implementation and found that fortunately, Antoine Pitrou already addressed this challenge (10 years ago!) by introducing an `_IterationGuard` context manager to the implementation, which delays mutation while an iteration is in progress.

I asked for confirmation and @pitrou agreed that these notes could be removed:
python@c1baa601e2b558deb690edfdf334fceee3b03327GH-commitcomment-39514438.
(cherry picked from commit 1642c0e)

Co-authored-by: Daniel Fortunov <asqui@users.noreply.github.com>
@bedevere-bot bedevere-bot removed the needs backport to 3.8 only security fixes label Jun 10, 2020
@bedevere-bot
Copy link

GH-20792 is a backport of this pull request to the 3.8 branch.

pitrou pushed a commit to pitrou/cpython that referenced this pull request Jun 10, 2020
…ythonGH-20687)

The doccumentation at https://docs.python.org/3.10/library/weakref.html cautions that the `WeakKeyDictionary` and `WeakValueDictionary` are susceptible to the problem of dictionary mutation during iteration.

These notes present the user with a problem that has no easy solution.

I dug into the implementation and found that fortunately, Antoine Pitrou already addressed this challenge (10 years ago!) by introducing an `_IterationGuard` context manager to the implementation, which delays mutation while an iteration is in progress.

I asked for confirmation and @pitrou agreed that these notes could be removed:
python@c1baa601e2b558deb690edfdf334fceee3b03327GH-commitcomment-39514438.
(cherry picked from commit 1642c0e)

Co-authored-by: Daniel Fortunov <asqui@users.noreply.github.com>
@bedevere-bot
Copy link

GH-20793 is a backport of this pull request to the 3.7 branch.

miss-islington added a commit that referenced this pull request Jun 10, 2020
)

The doccumentation at https://docs.python.org/3.10/library/weakref.html cautions that the `WeakKeyDictionary` and `WeakValueDictionary` are susceptible to the problem of dictionary mutation during iteration.

These notes present the user with a problem that has no easy solution.

I dug into the implementation and found that fortunately, Antoine Pitrou already addressed this challenge (10 years ago!) by introducing an `_IterationGuard` context manager to the implementation, which delays mutation while an iteration is in progress.

I asked for confirmation and @pitrou agreed that these notes could be removed:
c1baa601e2b558deb690edfdf334fceee3b03327GH-commitcomment-39514438
(cherry picked from commit 1642c0e)

Co-authored-by: Daniel Fortunov <asqui@users.noreply.github.com>
miss-islington pushed a commit that referenced this pull request Jun 10, 2020
…H-20687) (GH-20793)

The doccumentation at https://docs.python.org/3.10/library/weakref.html cautions that the `WeakKeyDictionary` and `WeakValueDictionary` are susceptible to the problem of dictionary mutation during iteration.

These notes present the user with a problem that has no easy solution.

I dug into the implementation and found that fortunately, Antoine Pitrou already addressed this challenge (10 years ago!) by introducing an `_IterationGuard` context manager to the implementation, which delays mutation while an iteration is in progress.

I asked for confirmation and @pitrou agreed that these notes could be removed:
c1baa601e2b558deb690edfdf334fceee3b03327GH-commitcomment-39514438.
(cherry picked from commit 1642c0e)

Co-authored-by: Daniel Fortunov <asqui@users.noreply.github.com>

Automerge-Triggered-By: @pitrou
miss-islington pushed a commit that referenced this pull request Jun 10, 2020
…H-20687) (GH-20792)

The doccumentation at https://docs.python.org/3.10/library/weakref.html cautions that the `WeakKeyDictionary` and `WeakValueDictionary` are susceptible to the problem of dictionary mutation during iteration.

These notes present the user with a problem that has no easy solution.

I dug into the implementation and found that fortunately, Antoine Pitrou already addressed this challenge (10 years ago!) by introducing an `_IterationGuard` context manager to the implementation, which delays mutation while an iteration is in progress.

I asked for confirmation and @pitrou agreed that these notes could be removed:
c1baa601e2b558deb690edfdf334fceee3b03327GH-commitcomment-39514438.
(cherry picked from commit 1642c0e)

Co-authored-by: Daniel Fortunov <asqui@users.noreply.github.com>

Automerge-Triggered-By: @pitrou
@asqui asqui deleted the remove-weakref-dict-warnings-bpo-31254 branch June 10, 2020 20:45
arun-mani-j pushed a commit to arun-mani-j/cpython that referenced this pull request Jul 21, 2020
…nGH-20687)

The doccumentation at https://docs.python.org/3.10/library/weakref.html cautions that the `WeakKeyDictionary` and `WeakValueDictionary` are susceptible to the problem of dictionary mutation during iteration.

These notes present the user with a problem that has no easy solution.

I dug into the implementation and found that fortunately, Antoine Pitrou already addressed this challenge (10 years ago!) by introducing an `_IterationGuard` context manager to the implementation, which delays mutation while an iteration is in progress.

I asked for confirmation and @pitrou agreed that these notes could be removed:
python@c1baa60#commitcomment-39514438
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants