mavutil: fix mavtcp.recv() crashing when autoreconnect is disabled

[khancyr]

recv() called self.reconnect() when self.port was None, but reconnect() is a no-op unless autoreconnect=True was passed to the constructor. With the default autoreconnect=False, a lost connection left self.port as None and the very next self.port.recv(n) call crashed with AttributeError instead of a clear "no data" result.

write() already has this same guard a few lines down (checks self.port is None after attempting reconnect and returns early); recv() was just missing its equivalent.

Now returns b"" (matching the existing "no data available right now" convention on the EAGAIN/EWOULDBLOCK exception path a few lines below) when the socket is still unset after the reconnect attempt.

Done with Claude.
tested with

#!/usr/bin/env python3

"""
test that mavtcp.recv() doesn't crash when the socket is gone and
autoreconnect is disabled (the default)
"""

import socket
import unittest

from pymavlink import mavutil


class MavtcpRecvNoAutoreconnectTest(unittest.TestCase):
    '''recv() must not raise when self.port is None and reconnect() is a no-op'''

    def test_recv_returns_empty_when_port_is_none(self):
        listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        listener.bind(('127.0.0.1', 0))
        listener.listen(1)
        port = listener.getsockname()[1]

        conn = mavutil.mavtcp('127.0.0.1:%d' % port, autoreconnect=False)
        try:
            # simulate a lost connection: no autoreconnect, so reconnect()
            # inside recv() is a no-op and self.port stays None
            conn.port.close()
            conn.port = None

            data = conn.recv()  # must not raise AttributeError
            self.assertEqual(data, b"")
        finally:
            conn.close()
            listener.close()


if __name__ == '__main__':
    unittest.main()

[peterbarker]

Please commit the test

[khancyr]

@peterbarker update with the fd closing and some tests

[peterbarker]

Unfortunately this does appear to break some things:

  ⎿  mavutil.py
       ● 1303 [correctness] Returning b"" whenever the socket is gone turns a permanently dead link (autoreconnect=False) into a silent infinite hang instead of a reported error.
       ● 1284 [correctness] close() now sets self.port = None, which makes close() reversible for autoreconnect connections: any later recv()/write() hits the `if self.port is None:
                            self.reconnect()` path and silently opens a brand-new TCP connection to the peer.
       ● 1285 [correctness] close() clears self.fd but reconnect()/do_connect() never restore it, so a reconnected mavtcp is left with fd=None and mavfile.select() stops waiting on the
                            socket.
       ● 1299 [correctness] With autoreconnect=False the guarded state (self.port is None on a live object) is only reachable via this same PR's new close(), so the guard largely
                            protects against a condition the PR itself introduces rather than the one the title describes.
       ● 1299 [cleanup]     The new recv() guard is a copy of the block write() already has at 1320-1326, minus write()'s try/except, so recv() still crashes on the autoreconnect=True
                            path the PR claims to fix.
       ● 1334 [cleanup]     reconnect() re-implements the teardown that close() now owns (port.close(); port=None) but does not clear self.fd, leaving the two paths inconsistent after
                            this diff — it should just call self.close().
       ● 1285 [cleanup]     Clearing self.fd on close() is applied only to mavtcp, while mavserial/mavudp/mavmcast/mavtcpin close() all still leave a stale fd — a per-class special case
                            where a shared convention is needed.
     tests/test_mavtcp.py
       ●   51 [cleanup]     test_recv_returns_empty_when_port_is_none_without_close hand-mutates self.port to None and its explanatory comment is factually wrong, so it proves only that
                            the guard survives external mutation — not that the claimed disconnect crash is fixed.
       ●   22 [cleanup]     No test constructs mavtcp with autoreconnect=True, so nothing covers the reconnect path that the new fd=None line actually breaks.
       ●   80 [correctness] test_recv_receives_data retries recv() 50 times with no sleep between attempts, so the whole retry budget can be consumed in microseconds before loopback
                            data is readable.
       ●   14 [correctness] get_free_port() closes the probe socket before setUp rebinds the port, a TOCTOU race that can make every test in the class error.
       ●   76 [cleanup]     test_recv_receives_data calls accept() directly on the non-blocking listen socket with no retry, bypassing the mavtcpin API.

Perhaps we do need to rethink setting that fd to None in this branch and do it separately?

[peterbarker]

Sorry, I think we also need a test for the "deliberately closed" cases.

[peterbarker]

Problem here is that we haven't checked whether we intentionally closed the port.

It only kind of worked by accident as it was, but now if you've marked the port as automatically-reconnect then when we go through this path we will reconnect even if the thing was closed on purpose.

We probably need some self._closed state to fix this.

[khancyr]

I have https://github.com/khancyr/pymavlink/tree/fix/mavtcp-close-is-final as follow up. I just didn't play with it yet ...