Fixed thread leak issue #2143 #2225
Conversation
WalkthroughRefactors SessionShutdownObserver to store a callback, revise polling/stop behavior, and add timeout-aware stopping. Updates WebRTC worker to treat "disconnected" as terminal, guard peer-connection close, stop processors, and explicitly stop and clear the shutdown observer during shutdown. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant ICE as ICE State Monitor
participant PC as RTCPeerConnection
participant WRK as WebRTC Worker
participant PROC as Media Processors
participant OBS as SessionShutdownObserver
ICE->>WRK: onIceStateChange(state)
alt state in {failed, closed, disconnected}
WRK->>PROC: stop()
WRK->>WRK: stop()
opt close peer connection
WRK->>PC: close() (guarded, try/except)
end
WRK->>OBS: stop(timeout)
WRK->>WRK: clear OBS reference
else other states
WRK->>WRK: no-op
end
sequenceDiagram
autonumber
participant OBS as SessionShutdownObserver
participant TH as Polling Thread
participant APP as AppSession Registry
participant CB as Stored Callback
OBS->>OBS: __init__(callback)->store as _callback
OBS->>TH: start polling
loop until stop flag
TH->>APP: check session presence/requested shutdown
alt removed or requested shutdown
TH->>TH: break loop
else present
TH->>TH: sleep/poll
end
end
TH->>OBS: set stop flag
TH->>CB: invoke _callback() (try/except)
OBS->>TH: stop(timeout) (if external caller)
alt join within timeout
OBS->>OBS: clear polling_thread
else timeout
OBS->>OBS: warn, clear polling_thread
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)streamlit_webrtc/shutdown.py (1)
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
streamlit_webrtc/shutdown.py (1)
66-68: Consider removing redundantis_alive()check.The
is_alive()check beforejoin()is generally safe but potentially introduces a TOCTOU (time-of-check to time-of-use) race. The thread could terminate between theis_alive()check and thejoin()call. However, callingjoin()on an already-finished thread is harmless and returns immediately, so this check may be unnecessary.Consider simplifying to:
- if self._polling_thread.is_alive(): - try: - self._polling_thread.join(timeout=timeout) - except Exception: - logger.exception("Failed to join ShutdownPolling thread") + try: + self._polling_thread.join(timeout=timeout) + except Exception: + logger.exception("Failed to join ShutdownPolling thread")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
streamlit_webrtc/shutdown.py(1 hunks)streamlit_webrtc/webrtc.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for Python formatting and linting (ruff format; ruff check --fix)
Run static type checking with mypy for Python code
Files:
streamlit_webrtc/webrtc.pystreamlit_webrtc/shutdown.py
🧬 Code graph analysis (2)
streamlit_webrtc/webrtc.py (6)
streamlit_webrtc/shutdown.py (1)
stop(57-72)streamlit_webrtc/process.py (2)
stop(44-48)stop(211-219)streamlit_webrtc/mix.py (1)
stop(194-202)streamlit_webrtc/receive.py (1)
stop(46-49)streamlit_webrtc/frontend/src/webrtc/index.ts (2)
state(50-85)console(176-187)streamlit_webrtc/frontend/src/webrtc/reducer.ts (1)
state(20-95)
streamlit_webrtc/shutdown.py (1)
streamlit_webrtc/webrtc.py (1)
stop(744-759)
🪛 Flake8 (7.2.0)
streamlit_webrtc/webrtc.py
[error] 547-547: indentation is not a multiple of 4
(E111)
[error] 547-547: over-indented
(E117)
[error] 548-548: indentation is not a multiple of 4
(E111)
🔇 Additional comments (2)
streamlit_webrtc/shutdown.py (2)
57-72: LGTM! Clean thread shutdown implementation.The updated
stop()method correctly implements bounded shutdown to prevent thread leaks:
- Guards against None
_polling_thread- Sets the stop event before joining
- Uses a timeout to prevent indefinite blocking
- Logs exceptions during join
- Clears the thread reference
This addresses the thread leak issue mentioned in PR #2143.
44-54: Enhanced log messages improve debuggability.The updated log messages provide clearer context for debugging shutdown behavior. Good improvement for observability.
|
can you add hacktoberfest label to it please |
kayush0712
force-pushed
the
fix-thread-leak
branch
from
9258295 to
9a653f8
Compare
|
hey @whitphx i did ruff check now can you please approve it with hacktoberfest label |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
streamlit_webrtc/webrtc.py (1)
543-558: Don’t treat “disconnected” as terminal; it can recover.Remove “disconnected” from the terminal set (keep “failed”, “closed”), or gate shutdown behind a grace timer.
- if ice_state in ("disconnected", "failed", "closed"): + if ice_state in ("failed", "closed"): logger.debug("ICE state=%s -> stopping WebRTC worker", ice_state)If you need a fallback, consider starting a timer on “disconnected” and stopping only if it remains non‑connected for N seconds.
🧹 Nitpick comments (2)
streamlit_webrtc/shutdown.py (1)
18-22: Break potential retain cycle; clear callback after use.Store the callback as Optional and null it after invocation to avoid lingering bound-method references.
-from typing import Callable, Union +from typing import Callable, Optional, Union @@ - def __init__(self, callback: Callback) -> None: + def __init__(self, callback: Callback) -> None: @@ - self._callback = callback + self._callback: Optional[Callback] = callback @@ - logger.debug("AppSession shutdown detected, executing callback.") - try: - self._callback() - except Exception as e: - logger.exception("Error in shutdown callback: %s", e) + logger.debug("AppSession shutdown detected, executing callback.") + cb = self._callback + self._callback = None + if cb: + try: + cb() + except Exception: + logger.exception("Error in shutdown callback")As per coding guidelines (mypy): Optional typing improves safety.
Also applies to: 58-63
streamlit_webrtc/webrtc.py (1)
768-772: Propagate timeout to shutdown observer for consistency.Use the same timeout passed to WebRtcWorker.stop() to bound the observer’s join as well.
- if self._session_shutdown_observer: - self._session_shutdown_observer.stop() + if self._session_shutdown_observer: + self._session_shutdown_observer.stop(timeout=timeout if timeout is not None else 1.0) self._session_shutdown_observer = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
streamlit_webrtc/shutdown.py(2 hunks)streamlit_webrtc/webrtc.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for Python formatting and linting (ruff format; ruff check --fix)
Run static type checking with mypy for Python code
Files:
streamlit_webrtc/webrtc.pystreamlit_webrtc/shutdown.py
🧬 Code graph analysis (2)
streamlit_webrtc/webrtc.py (5)
streamlit_webrtc/shutdown.py (1)
stop(64-78)streamlit_webrtc/mix.py (1)
stop(194-202)streamlit_webrtc/process.py (2)
stop(44-48)stop(211-219)streamlit_webrtc/receive.py (1)
stop(46-49)streamlit_webrtc/frontend/src/webrtc/index.ts (3)
state(50-85)console(176-187)pc(80-83)
streamlit_webrtc/shutdown.py (1)
streamlit_webrtc/webrtc.py (2)
callback(583-592)stop(752-771)
🔇 Additional comments (1)
streamlit_webrtc/shutdown.py (1)
68-77: Good: avoid self-join deadlock.Skipping join when called from the polling thread itself prevents deadlock and hangs.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
streamlit_webrtc/webrtc.py (1)
546-546: "disconnected" is a transient ICE state, not terminal.Per WebRTC spec, "disconnected" indicates temporary connectivity loss and may recover, unlike "failed" and "closed" which are terminal. Treating it as terminal can cause premature shutdowns.
Based on past review and WebRTC best practices.
Apply this diff to remove "disconnected" from terminal states:
- if ice_state in ("failed", "closed", "disconnected"): + if ice_state in ("failed", "closed"): logger.debug("ICE state=%s -> scheduling stop of WebRTC worker", ice_state)
🧹 Nitpick comments (2)
streamlit_webrtc/webrtc.py (2)
554-554: Remove redundant exception formatting.
logger.exception()automatically includes the stack trace viaexc_info=True, so the%s, eis redundant and could cause confusion.Apply this diff:
- logger.exception("Error while stopping WebRTC worker: %s", e) + logger.exception("Error while stopping WebRTC worker")
765-768: Pass timeout parameter to shutdown observer for consistency.The
stop()method accepts atimeoutparameter but doesn't forward it to_session_shutdown_observer.stop(), which also accepts a timeout. For consistency and to respect the caller's timeout intent, pass the timeout through.Apply this diff:
# 💡 Explicitly stop shutdown observer here if self._session_shutdown_observer: - self._session_shutdown_observer.stop() + self._session_shutdown_observer.stop(timeout=timeout if timeout else 1.0) self._session_shutdown_observer = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
streamlit_webrtc/webrtc.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff for Python formatting and linting (ruff format; ruff check --fix)
Run static type checking with mypy for Python code
Files:
streamlit_webrtc/webrtc.py
🧬 Code graph analysis (1)
streamlit_webrtc/webrtc.py (4)
streamlit_webrtc/shutdown.py (1)
stop(64-78)streamlit_webrtc/mix.py (1)
stop(194-202)streamlit_webrtc/process.py (2)
stop(44-48)stop(211-219)streamlit_webrtc/receive.py (1)
stop(46-49)
🔇 Additional comments (1)
streamlit_webrtc/webrtc.py (1)
750-750: Good addition for debugging.The debug log at the start of
stop()helps trace the shutdown flow.
|
Thanks, this repo has already tagged with a repo-wide "hacktoberfest" topic so this should be counted as a contribution when merged. |
| self._polling_thread.join() | ||
|
|
||
| # 🔑 FIX: do not join current thread | ||
| if threading.current_thread() is not self._polling_thread: |
There was a problem hiding this comment.
done!
kindly merge it
thank you so much for giving my first open source project
There was a problem hiding this comment.
I meant to run make format and commit the formatted code to this PR so that the CI passes. Thanks!
There was a problem hiding this comment.
but there is no new commit in this PR and the CI still fails 🤔
kayush0712
force-pushed
the
fix-thread-leak
branch
from
5cac8de to
fec36f3
Compare
|
is it okay now? |
|
Plz don't add unnecessary changes on the frontend code... |
|
sorry for this i am very new for all of this . i am trying to learn |
kayush0712
force-pushed
the
fix-thread-leak
branch
from
1b5a274 to
6d6a00d
Compare
|
hey @whitphx can you check it now |
|
hey @whitphx sorry to ping you but is it okay now ? |
|
@kayush0712 Thanks, all tests passed! (The remaining CI error is a problem on our side) |
Summary by CodeRabbit