Skip to content

fix: fastapi memory leak only #3688

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Fixed

- `opentelemetry-instrumentation-fastapi`: Fix memory leak in `uninstrument_app()` by properly removing apps from the tracking set
Copy link
Member

Choose a reason for hiding this comment

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

Need to move that to # Unreleased section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to #Unreleased section

([#3688](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3688), [#3683](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3683))
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added correct link to PR


- `opentelemetry-instrumentation`: Fix dependency conflict detection when instrumented packages are not installed by moving check back to before instrumentors are loaded. Add "instruments-any" feature for instrumentations that target multiple packages.
([#3610](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/3610))
- infra(ci): Fix git pull failures in core contrib test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ def client_response_hook(span: Span, scope: dict[str, Any], message: dict[str, A
import logging
import types
from typing import Collection, Literal
from weakref import WeakSet as _WeakSet

import fastapi
from starlette.applications import Starlette
Expand Down Expand Up @@ -358,6 +359,11 @@ def uninstrument_app(app: fastapi.FastAPI):
app.middleware_stack = app.build_middleware_stack()
app._is_instrumented_by_opentelemetry = False

# Remove the app from the set of instrumented apps to avoid calling uninstrument twice
# if the instrumentation is later disabled or such
# Use discard to avoid KeyError if already GC'ed
_InstrumentedFastAPI._instrumented_fastapi_apps.discard(app)

def instrumentation_dependencies(self) -> Collection[str]:
return _instruments

Expand Down Expand Up @@ -388,7 +394,11 @@ def _instrument(self, **kwargs):
fastapi.FastAPI = _InstrumentedFastAPI

def _uninstrument(self, **kwargs):
for instance in _InstrumentedFastAPI._instrumented_fastapi_apps:
# Create a copy of the set to avoid RuntimeError during iteration
instances_to_uninstrument = list(
_InstrumentedFastAPI._instrumented_fastapi_apps
)
for instance in instances_to_uninstrument:
self.uninstrument_app(instance)
_InstrumentedFastAPI._instrumented_fastapi_apps.clear()
fastapi.FastAPI = self._original_fastapi
Expand All @@ -406,7 +416,8 @@ class _InstrumentedFastAPI(fastapi.FastAPI):
_http_capture_headers_sanitize_fields: list[str] | None = None
_exclude_spans: list[Literal["receive", "send"]] | None = None

_instrumented_fastapi_apps = set()
# Track instrumented app instances using weak references to avoid GC leaks
_instrumented_fastapi_apps = _WeakSet()
_sem_conv_opt_in_mode = _StabilityMode.DEFAULT

def __init__(self, *args, **kwargs):
Expand All @@ -426,10 +437,6 @@ def __init__(self, *args, **kwargs):
)
_InstrumentedFastAPI._instrumented_fastapi_apps.add(self)

def __del__(self):
if self in _InstrumentedFastAPI._instrumented_fastapi_apps:
_InstrumentedFastAPI._instrumented_fastapi_apps.remove(self)


def _get_route_details(scope):
"""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@

# pylint: disable=too-many-lines

import gc as _gc
import unittest
import weakref as _weakref
from contextlib import ExitStack
from timeit import default_timer
from unittest.mock import Mock, call, patch
Expand Down Expand Up @@ -1400,6 +1402,16 @@ def test_mark_span_internal_in_presence_of_span_from_other_framework(self):
)


class TestFastAPIGarbageCollection(unittest.TestCase):
def test_fastapi_app_is_collected_after_instrument(self):
app = fastapi.FastAPI()
otel_fastapi.FastAPIInstrumentor().instrument_app(app)
app_ref = _weakref.ref(app)
del app
_gc.collect()
self.assertIsNone(app_ref())


@patch.dict(
"os.environ",
{
Expand Down