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

gh-106739: Add rtype_cache to warnings.warn message when leaked objects found #106740

Merged

Conversation

shailshouryya
Copy link
Contributor

@shailshouryya shailshouryya commented Jul 14, 2023

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

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

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Jul 14, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@shailshouryya shailshouryya changed the title Add rtype_cache to warnings.warn message when leaked objects found gh-106739: Add rtype_cache to warnings.warn message when leaked objects found Jul 14, 2023
@shailshouryya
Copy link
Contributor Author

shailshouryya commented Jul 14, 2023

I'm not sure what the corresponding news entry should be for this in `Misc/NEWS` - would this go under `Core and Builtins`?

Or should we apply the skip news label here? I can't seem to modify labels here, though, so someone else might need to add the skip news label for me in that case. (I'm guessing this is because I don't have high enough permissions to modify labels here)

UPDATE: looked through the blurb_it web app again and realized this change probably falls under the Library category of changes (since this affects the Lib/multiprocessing/resource_tracker.py module), so I added an entry for Library using the blurb_it web app. Let me know if this is correct, or if I should modify this.

@shailshouryya
Copy link
Contributor Author

@iritkatriel tagging you here since I saw you were also involved in #104090 and the pull requests associated with that issue.

Let me know what you think of this when you get the chance! 🤓 (Or let me know if I should tag someone else instead)

@iritkatriel
Copy link
Member

CC @pitrou .

@pitrou
Copy link
Member

pitrou commented Jul 24, 2023

@shailshouryya Can you show an example of the improved warning message?

@shailshouryya
Copy link
Contributor Author

Can you show an example of the improved warning message?

Sure! Prior to these changes, the warning message looked like

/Users/username/cpython/Lib/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 6 leaked semaphore objects to clean up at shutdown

and after these changes, the warning message looks like

/Users/username/cpython/Lib/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 6 leaked semaphore objects to clean up at shutdown: {'/mp-x7qw67gg', '/mp-bwptfpzm', '/mp-evehw7gr', '/mp-qxl5kdci', '/mp-a90ggt9g', '/mp-pi9c59yl'}

I got these outputs by running ./python.exe -m test test_concurrent_futures --verbose on this branch before these changes and after these changes.

Full command to rebuild and test (on macOS)
GDBM_CFLAGS="-I$(dirname $(dirname $(which port)))/include" \
GDBM_LIBS="-L$(dirname $(dirname $(which port)))/lib -lgdbm" \
./configure --with-pydebug  && make regen-global-objects && make -s -j3 \
&& ./python.exe -m test test_concurrent_futures --verbose

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, thank you @shailshouryya

shailshouryya and others added 5 commits July 25, 2023 18:47
Adding the `rtype_cache` to the `warnings.warn` message improves the
previous, somewhat vague message from

```
/Users/username/cpython/Lib/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 6 leaked semaphore objects to clean up at shutdown
```

to

```
/Users/username/cpython/Lib/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 6 leaked semaphore objects to clean up at shutdown: {'/mp-yor5cvj8', '/mp-10jx8eqr', '/mp-eobsx9tt', '/mp-0lml23vl', '/mp-9dgtsa_m', '/mp-frntyv4s'}
```
The previously used f-string format did not properly interpolate the
variables, and resulted in an output such as the following:

```
/Users/username/cpython/Lib/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 6 leaked {rtype} objects to clean up at shutdown: {rtype_cache}
```

Resource: https://stackoverflow.com/questions/49416042/how-to-write-an-f-string-on-multiple-lines-without-introducing-unintended-whites
@pitrou pitrou force-pushed the resource_tracker_message_improvement branch from 1611dd4 to 17f4769 Compare July 25, 2023 16:47
@pitrou pitrou enabled auto-merge (squash) July 25, 2023 16:49
@pitrou pitrou merged commit fabcbe9 into python:main Jul 25, 2023
jtcave pushed a commit to jtcave/cpython that referenced this pull request Jul 27, 2023
…aked objects found (python#106740)

Adding the `rtype_cache` to the `warnings.warn` message improves the
previous, somewhat vague message from

```
/Users/username/cpython/Lib/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 6 leaked semaphore objects to clean up at shutdown
```

to

```
/Users/username/cpython/Lib/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 6 leaked semaphore objects to clean up at shutdown: {'/mp-yor5cvj8', '/mp-10jx8eqr', '/mp-eobsx9tt', '/mp-0lml23vl', '/mp-9dgtsa_m', '/mp-frntyv4s'}
```

---------

Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants