bpo-32574: asyncio.Queue, put() leaks memory#5208
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
|
I sign the CLA some minutes ago, I am waiting for the verification. |
| try: | ||
| self._putters.remove(putter) | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
So I applied this patch and changed pass to raise. Nothing happened, test_cancelled_putters_not_being_held_in_self_putters passed just fine. Putting a print() before raise showed that this except block isn't triggered by the test at all.
All in all, I don't understand this fix. Please make sure that the test (a) triggers the code you change; (b) there's a clear comment in the code explaining why/what it's doing.
There was a problem hiding this comment.
Added a refactor of the test with the proper comments, comments to the bugfix explaining it, and added also another test to ensure that the except block is correctly executed.
The requested changes are really similar to the merged code of the following link, it is the same memory leak but in the getters instead of the putters.
c62f0cb#diff-3947468b604f664993c24d2b5560635c
I think that now the code is much more clear and easy to understand, thx for the correction.
|
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 |
- Added comments explaining the problem to the test and the bugfix. - Added one test to ensure that the ValueError is silenced. - Refactor of the other test.
|
I have made the requested changes; please review again |
|
Looks like now all code paths are covered by unit tests. |
| # Clean self._putters from canceled putters. | ||
| self._putters.remove(putter) | ||
| except ValueError: | ||
| # The putter could be removed from self._putters by a previous get_nowait call. |
There was a problem hiding this comment.
This line is longer than 79 characters, right? Please PEP8 the code.
1st1
left a comment
1st1
left a comment
There was a problem hiding this comment.
The code looks OK, but please fix the code style—PEP8.
|
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. Ups, I had the entire work environment with the right margin at 120, sorry, fixed code style. |
|
Thank you, José! |
5 participants
https://bugs.python.org/issue32574
https://bugs.python.org/issue32574