Skip to content

fastapi: Fix uninstrument memory leak#3683

Closed
artemziborev wants to merge 10 commits intoopen-telemetry:mainfrom
artemziborev:fix/uninstrument-memory-leak
Closed

fastapi: Fix uninstrument memory leak#3683
artemziborev wants to merge 10 commits intoopen-telemetry:mainfrom
artemziborev:fix/uninstrument-memory-leak

Conversation

@artemziborev
Copy link
Copy Markdown
Contributor

@artemziborev artemziborev commented Aug 6, 2025 •
edited
Loading

Description

Fixes a memory leak in the FastAPIInstrumentor.uninstrument_app() method, where the FastAPI app instance was not being removed from the internal _instrumented_apps list. This caused a retained reference even after uninstrumentation, potentially resulting in memory usage growth over time.

Fixes #3683

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Added a regression test to ensure that after uninstrument_app() is called, the app is no longer present in the _instrumented_apps list.

Steps to reproduce:

  1. Instrument a FastAPI app via FastAPIInstrumentor.instrument_app()
  2. Uninstrument it via uninstrument_app()
  3. Assert that the app is no longer tracked (i.e., removed from _instrumented_apps)
  4. Confirm no side effects in other instrumentation logic

Tested using:

  • Python 3.x
  • tox
  • pytest

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated (not required)

@artemziborev artemziborev requested a review from a team as a code owner August 6, 2025 05:18
@artemziborev
Copy link
Copy Markdown
Contributor Author

artemziborev commented Aug 6, 2025

Hi! All checks are green — would love to get a review when you have time 🙌 Let me know if you'd like anything changed!

@emdneto emdneto moved this to Ready for review in Python PR digest Aug 7, 2025
emdneto
emdneto reviewed Aug 7, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@emdneto emdneto left a comment

Choose a reason for hiding this comment

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

Thanks. SGTM. We don't need the attributes fix for that PR. I suggest we minimize the scope of this PR only to the actual fix for FastAPI.

Comment thread CHANGELOG.md
### Fixed

- `opentelemetry-instrumentation-fastapi`: Fix memory leak in `uninstrument_app()` method by properly removing apps from the tracking set
([#XXXX](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/XXXX))
Copy link
Copy Markdown
Member

@emdneto emdneto Aug 7, 2025

Choose a reason for hiding this comment

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

Suggested change
([#XXXX](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/XXXX))
([#3683](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3683))

Comment thread instrumentation/opentelemetry-instrumentation-fastapi/demo_memory_leak_fix.py
Copy link
Copy Markdown
Member

@emdneto emdneto Aug 7, 2025

Choose a reason for hiding this comment

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

thank you for the repro, but can you instead create an issue with that instead and remove from the PR?

Comment thread instrumentation/opentelemetry-instrumentation-fastapi/tests/test_fastapi_memory_leak.py
Copy link
Copy Markdown
Member

@emdneto emdneto Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe a better way to test would be just to add this to somewhere in test_fastapi_instrumentation.py:

    def test_fastapi_app_is_collected_after_uninstrument(self):
        import gc
        import weakref
        app = fastapi.FastAPI()
        otel_fastapi.FastAPIInstrumentor.instrument_app(app)
        app_ref = weakref.ref(app)
        del app
        gc.collect()
        self.assertIsNone(app_ref())

Copy link
Copy Markdown
Contributor

@anuraaga anuraaga Aug 7, 2025

Choose a reason for hiding this comment

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

Just a note related to #3683 (comment), it's probably better to remove uninstrument_app from this test, since it should still not leak

Copy link
Copy Markdown
Member

@emdneto emdneto Aug 7, 2025

Choose a reason for hiding this comment

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

Makes sense @anuraaga

@anuraaga
Copy link
Copy Markdown
Contributor

anuraaga commented Aug 7, 2025

Thanks @artemziborev - I had randomly noticed the same issue in starlette and fixed by using a WeakSet, which would allow avoiding all explicit remove()

b9a78e7#diff-a4901479b7a21ad4f5d45a7f3e47b2ada3f9d8ae4ae7f9e68c2001e8b8067611R303

I think it's more reliable to use WeakSet and recommend updating here too since there is still the chance of an app being GC'd without uninstrument being called. This instrumentation has the same __del__ hook as starlette used to

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py#L429

but it's essentially dead code since __del__ won't be called as long as the set() has a reference to it anyways.

@emdneto
Copy link
Copy Markdown
Member

emdneto commented Aug 7, 2025

Thanks @artemziborev - I had randomly noticed the same issue in starlette and fixed by using a WeakSet, which would allow avoiding all explicit remove()

b9a78e7#diff-a4901479b7a21ad4f5d45a7f3e47b2ada3f9d8ae4ae7f9e68c2001e8b8067611R303

I think it's more reliable to use WeakSet and recommend updating here too since there is still the chance of an app being GC'd without uninstrument being called. This instrumentation has the same __del__ hook as starlette used to

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/instrumentation/opentelemetry-instrumentation-fastapi/src/opentelemetry/instrumentation/fastapi/__init__.py#L429

but it's essentially dead code since __del__ won't be called as long as the set() has a reference to it anyways.

Yup. +1 if the author is open to making this change

@emdneto emdneto changed the title Fix/uninstrument memory leak fastapi: Fix uninstrument memory leak Aug 7, 2025
@xrmx xrmx moved this from Ready for review to Reviewed PR that needs fixing in Python PR digest Aug 11, 2025
artemziborev added a commit to artemziborev/opentelemetry-python-contrib that referenced this pull request Aug 12, 2025
@artemziborev
docs(changelog): add FastAPI uninstrument memory leak fix with PR ref…
7218064 
…erences (open-telemetry#3683)
@artemziborev
Copy link
Copy Markdown
Contributor Author

artemziborev commented Aug 12, 2025

Thanks for the reviews!

Per feedback to minimize scope to FastAPI only (and drop unrelated changes), I opened a new, focused PR: #3688.

That PR includes:

  • WeakSet for tracking instrumented FastAPI apps (prevents leaks even without calling uninstrument_app).
  • A GC-based test moved into test_fastapi_instrumentation.py (no uninstrument_app in the test).
  • Changelog entry updated accordingly.
  • Removed demo and any non-FastAPI edits.

Closing this PR in favor of #3688. 🙏

@github-project-automation github-project-automation Bot moved this from Reviewed PR that needs fixing to Done in Python PR digest Aug 12, 2025
xrmx added a commit that referenced this pull request Aug 23, 2025
@artemziborev @emdneto @xrmx
fix: fastapi memory leak only (#3688)
5fa222f 
* fix(fastapi-instrumentation): properly remove app from instrumented list to avoid memory leaks

* fix(fastapi-instrumentation): fix tests and finalize memory leak fix

* docs(changelog): add FastAPI uninstrument memory leak fix with PR references (#3683)

* test(fastapi): add GC-based app collection test; refactor tracking to WeakSet and safe discard

* docs(changelog): reference FastAPI memory leak fix PR (#3688)

* refactor(fastapi): drop __del__ as WeakSet handles cleanup

* chore(fastapi): formatting after ruff auto-fix

* chore(test-fastapi): ruff import order

* Update comment to clarify purpose of removing app from WeakSet

* fix: codereview comments

* Apply suggestions from code review

---------

Co-authored-by: Emídio Neto <[email protected]>
Co-authored-by: Riccardo Magliocchetti <[email protected]>
sightseeker added a commit to sightseeker/opentelemetry-python-contrib that referenced this pull request Mar 11, 2026
@sightseeker @emdneto @xrmx
fix: fastapi memory leak only (open-telemetry#3688)
2951225 
* fix(fastapi-instrumentation): properly remove app from instrumented list to avoid memory leaks

* fix(fastapi-instrumentation): fix tests and finalize memory leak fix

* docs(changelog): add FastAPI uninstrument memory leak fix with PR references (open-telemetry#3683)

* test(fastapi): add GC-based app collection test; refactor tracking to WeakSet and safe discard

* docs(changelog): reference FastAPI memory leak fix PR (open-telemetry#3688)

* refactor(fastapi): drop __del__ as WeakSet handles cleanup

* chore(fastapi): formatting after ruff auto-fix

* chore(test-fastapi): ruff import order

* Update comment to clarify purpose of removing app from WeakSet

* fix: codereview comments

* Apply suggestions from code review

---------

Co-authored-by: Emídio Neto <[email protected]>
Co-authored-by: Riccardo Magliocchetti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emdneto emdneto emdneto left review comments

+1 more reviewer

@anuraaga anuraaga anuraaga left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Python PR digest
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@artemziborev @anuraaga @emdneto @xrmx