async-lru icon indicating copy to clipboard operation
async-lru copied to clipboard

Fix memory leaks related to exception handling.

Open davidmfrey opened this issue 6 years ago • 9 comments

What do these changes do?

This PR fixes a few memory leaks related to exception handling.

The problems:

  1. When cache_exceptions == True, the same cached exception is re-raised every time there is a hit. This causes the current traceback to be appended to the traceback on the exception (including references to local objects in the frame). Over time this leads to a very large traceback that is retained on the cached exceptions. See the graph here.
  2. When cache_exceptions == False and the wrapped function repeatedly raises an exception, a reference to the previously cached exception is retained in the traceback of the new exception. See the graph here
  3. The tests performed on exception behaviour were not quite accurate. By calling asyncio.gather() all of the tested coroutines execute the first block of decorator code before the user-defined coroutine produces an exception. This skips much of the code path for exceptional behaviour.

The solutions:

  1. Rather than reraise the original cached exception, raise a copy of it. This prevents the traceback on the original exception from growing every time it is hit. A test has been added to verify the length of the traceback after repeated cache hits.
  2. Setting the local exc variable to None releases the reference and allows it to be garbage collected. A test has been added to verify that the exc local variable exists and is set to None in the traceback of the exception. There is a function traceback.clear_frames(), but it doesn't seem to completely clear all the local variables from the traceback.
  3. The tests have been rewritten to allow each exceptional coroutine to finish completely before starting the next one. This allows the complete code path to be covered.

I also added a TODO in the code where cached exceptions can be returned to the user even if cache_exceptions == False and a unit test for the case that is marked xfail. That fix is out of scope for this PR.

Are there changes in behavior for the user?

  • Memory will stop leaking :-)
  • Tracebacks of cached exceptions will be modified from the previous behaviour.

Related issue number

Checklist

  • [x] I think the code is well written
  • [x] Unit tests for the changes exist

davidmfrey avatar Feb 22 '20 20:02 davidmfrey

Codecov Report

Merging #199 into master will not change coverage by %. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #199   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          136       136           
  Branches        24        24           
=========================================
  Hits           136       136           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 43e3975...e54afa8. Read the comment docs.

codecov[bot] avatar Feb 22 '20 20:02 codecov[bot]

This pull request introduces 1 alert when merging 24b3212ec02e59ac05911b0a5e467ee9f20fac20 into 43e397590749a47cb1e1df174bf25c6c9024d825 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Feb 22 '20 20:02 lgtm-com[bot]

This pull request introduces 1 alert when merging ca583dcd6296d84fef92d866f8ffcb16b094f058 into 43e397590749a47cb1e1df174bf25c6c9024d825 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Feb 22 '20 21:02 lgtm-com[bot]

This pull request introduces 1 alert when merging 2d5fc2755e5338cdfa3e16e30e5005ca9585a286 into 43e397590749a47cb1e1df174bf25c6c9024d825 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Feb 22 '20 22:02 lgtm-com[bot]

This pull request introduces 1 alert when merging e54afa824e277fdc8d476bd647cb2e457bacef76 into 43e397590749a47cb1e1df174bf25c6c9024d825 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

lgtm-com[bot] avatar Feb 23 '20 23:02 lgtm-com[bot]

any chance of getting this or some variation merged?

cancan101 avatar Oct 26 '20 22:10 cancan101

Btw I am seeing issues with this code: __init__() missing 1 required keyword-only argument: 'request' image for a httpx.ReadTimeout exception.

cancan101 avatar Oct 27 '20 17:10 cancan101

ex = httpx.RequestError('', request=1)
copy.copy(ex)

yields:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/alex/.pyenv/versions/3.8.2/lib/python3.8/copy.py", line 102, in copy
    return _reconstruct(x, None, *rv)
  File "/Users/alex/.pyenv/versions/3.8.2/lib/python3.8/copy.py", line 264, in _reconstruct
    y = func(*args)
TypeError: __init__() missing 1 required keyword-only argument: 'request'

cancan101 avatar Oct 27 '20 17:10 cancan101

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


matt frey seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Nov 16 '20 10:11 CLAassistant

The memory leak is not related to the library directly. It was fixed by asyncio from Python 3.11 IIRC

asvetlov avatar Feb 19 '23 00:02 asvetlov