Skip to content

StreamInterface: prevent socket/reader-thread leak on handshake failure #918

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.

Sign up for GitHub

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
nightjoker7 wants to merge 1 commit into
base: master
Choose a base branch
from
+22 −4
Open

StreamInterface: prevent socket/reader-thread leak on handshake failure #918

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 22 additions & 4 deletions meshtastic/stream_interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Stream Interface base class
"""
import contextlib
import io
import logging
import threading
Expand Down Expand Up @@ -61,9 +62,20 @@ def __init__( # pylint: disable=R0917

# Start the reader thread after superclass constructor completes init
if connectNow:
self.connect()
if not noProto:
self.waitForConfig()
try:
self.connect()
if not noProto:
self.waitForConfig()
except Exception:
# Handshake failed (timeout, config error, bad stream). The caller
# never receives a reference to this half-initialized instance, so
# they cannot call close() themselves. If we don't clean up here,
# the reader thread (already started by connect()) keeps running
# and the underlying stream/socket leaks — the leak compounds on
# every retry from the caller's reconnect loop.
with contextlib.suppress(Exception):
self.close()
raise
Comment on lines +65 to +78
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

New failure-path behavior (calling close() from __init__ when connect() / waitForConfig() raises) is important for leak prevention but currently isn’t covered by unit tests. Since there are existing tests for stream_interface.py, consider adding a test that forces connect() or waitForConfig() to raise and asserts cleanup occurs (e.g., close() is invoked and doesn’t raise when the thread wasn’t started).

Copilot generated this review using guidance from repository custom instructions.

def connect(self) -> None:
"""Connect to our radio
Expand Down Expand Up @@ -136,7 +148,13 @@ def close(self) -> None:
# reader thread to close things for us
self._wantExit = True
if self._rxThread != threading.current_thread():
self._rxThread.join() # wait for it to exit
try:
self._rxThread.join() # wait for it to exit
except RuntimeError:
Comment on lines 150 to +153
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

join() without a timeout can block indefinitely if the reader thread is stuck in a blocking _readBytes() (e.g., TCPInterface uses blocking socket.recv() with no timeout). With the new __init__ exception handler calling self.close(), a handshake timeout against a silent TCP peer can hang here and never re-raise the original exception. Consider using a bounded join (and letting transport-specific close() logic interrupt the read), or otherwise ensuring blocking reads are interrupted before joining.

Copilot uses AI. Check for mistakes.
# Thread was never started — happens when close() is invoked
# from a failed __init__ before connect() could spawn it.
# Nothing to join; safe to ignore.
pass
Comment on lines +156 to +157
Copy link

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

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

If connect() fails before the reader thread is started (e.g., serial write raises before _rxThread.start()), this RuntimeError path will skip joining but also won’t close the underlying stream. For SerialInterface (which opens the port before calling StreamInterface.__init__), this means the serial port can remain open even though close() was invoked. Consider explicitly closing self.stream (and setting it to None) when the thread was never started / is not alive, instead of just pass.

Suggested change
# Nothing to join; safe to ignore.
pass
# In that case the reader thread cannot run _disconnected(),
# so close the stream here to avoid leaking the resource.
if self.stream is not None:
with contextlib.suppress(Exception):
self.stream.close()
self.stream = None

Copilot uses AI. Check for mistakes.

def _handleLogByte(self, b):
"""Handle a byte that is part of a log message from the device."""
Expand Down
Loading