Skip to content

bpo-44310: added docs to functools to warn for memory leaks #26528

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

Closed
wants to merge 2 commits into from

Conversation

wouterdb
Copy link

@wouterdb wouterdb commented Jun 4, 2021

I implemented the easier fix for https://bugs.python.org/issue44310: documenting the unexpected behavior.

To actually change the behavior look a bit too tricky. WeakKeyDictionary does what is needed, but it doesn't update the LRU linked list, and offers no hook to do so. I could make an attempt to make the change, but I'm not sufficiently familiar with the python codebase to do it without confirmation that I can.

https://bugs.python.org/issue44310

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@wouterdb

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir labels Jun 4, 2021
Copy link
Contributor

@nanjekyejoannah nanjekyejoannah left a comment

Choose a reason for hiding this comment

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

Sounds sensible to especially document but I will also ping @pablogsal to confirm if this is a bug that needs solving or its acceptable behavior, so to speak.

@pablogsal
Copy link
Member

Thanks a lot @nanjekyejoannah for checking with me. @wouterdb I think it makes sense to mention this in the docs, but I would like to change the phrasing a bit

@rhettinger rhettinger self-assigned this Jun 4, 2021
@wouterdb
Copy link
Author

wouterdb commented Jun 7, 2021

Ok, thank you for looking into this so quickly.

I agree that the behavior is fully in-line with the rest of the python sdk (if a bit unexpected here) and that my proposed changes are not as well put as they could.

Do I understand correctly that this is now in the python team's capable hands and that it would not help for me to propose further changes?

@@ -144,6 +144,11 @@ The :mod:`functools` module defines the following functions:
functions with side-effects, functions that need to create distinct mutable
objects on each call, or impure functions such as time() or random().

The LRU cache takes string references to the arguments provided in the decorated callable
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
The LRU cache takes string references to the arguments provided in the decorated callable
The LRU cache takes strong references to the arguments provided in the decorated callable

@@ -144,6 +144,11 @@ The :mod:`functools` module defines the following functions:
functions with side-effects, functions that need to create distinct mutable
objects on each call, or impure functions such as time() or random().

The LRU cache takes string references to the arguments provided in the decorated callable
as well as the return value. Notice that this means that if a class method is decorated, the cache
will keep strong references to all arguments provided, including the instance itself (provided as the
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
will keep strong references to all arguments provided, including the instance itself (provided as the
will keep strong references to all arguments provided, including the instance itself (provided as first the

@rhettinger
Copy link
Contributor

Closing in favor of PR 26715

@rhettinger rhettinger closed this Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants