mavutil: tidy exceptions by khancyr · Pull Request #1252 · ArduPilot/pymavlink

khancyr · 2026-08-06T18:51:30Z

tidy broad exceptions
remove useless pass
select.error is alias OSError : use OSError to make analyzer happy

khancyr · 2026-08-06T18:52:24Z

            print(f"SSL Error: {e}")
            self.close()
            raise
+        except OSError as e:


just a reorder as oserror cover sslerror , so we need to handle them first

peterbarker · 2026-08-07T00:41:39Z

I do like the change, but there are several regressions.

I suggest maybe subset the easier ones to get them in first....

  ⎿  mavutil.py
       ●  994 [correctness] mavserial.__init__ narrows the fileno() guard to (AttributeError, serial.SerialException), but pySerial's Windows backend inherits io.RawIOBase.fileno(),
                            which raises io.UnsupportedOperation (an OSError/ValueError subclass, not SerialException) — so the exception now escapes. Codex independently flags the same
                            probe (and mavutil.py:1055) for ValueError from a closed file-like object. Same root cause at mavutil.py:1058.
       ● 1033 [correctness] mavserial.write narrows to (serial.SerialException, OSError), which no longer catches the bare `Exception` that FakeSerial.write deliberately raises,
                            breaking the force_connected / dead-device path.
       ●  132 [correctness] set_dialect only auto-regenerates a dialect module on ImportError, so a corrupt or partially written generated dialect no longer self-heals.
       ●  588 [correctness] param_fetch_one moves param_request_read_send out of the guarded block into an `else:` branch, so a numeric name that overflows the int16 param_index now
                            raises struct.error instead of falling back to a by-name request. Independently confirmed by Codex as the one structural (non-tuple) regression in the PR.
       ●  955 [correctness] set_close_on_exec, documented as "Ignore exceptions", narrowed from `except Exception` to `(ImportError, OSError)`, but fcntl.fcntl raises
                            ValueError/TypeError for bad fds. Codex flags the same gap independently.
       ●   29 [correctness] Top-level optional `from pymavlink.dialects.v10 import all` guard narrowed from `except Exception` to `except ImportError`, so any non-ImportError failure in
                            the generated module now aborts importing mavutil. Codex reaches the same conclusion and notes it contradicts the comment's stated best-effort intent.
       ● 2221 [cleanup]     auto_detect_serial_win32 narrowed from a bare `except:` to (ImportError, OSError), so non-ImportError/non-OSError failures in pySerial's win32 enumeration
                            now escape instead of degrading to an empty list.
       ● 1064 [cleanup]     mavserial.reset narrowed to (serial.SerialException, OSError), so non-OSError failures now escape reset() and re-escape write()'s dead-port handler,
                            violating its boolean failure contract. Codex flags the same, adding ValueError from `serial.Serial(...)` on an invalid baud as a second trigger.
       ● 1030 [cleanup]     A redundant `import serial` was added to the per-packet write() hot path, and serial.SerialException is already an OSError subclass so the tuple collapses to
                            `except OSError`.
       ● 2486 [cleanup]     `except OSError: raise` is a no-op handler left in place after the narrowing.

I'm particularly concerned about the win32 implications.

Replace handlers that say less than they mean, without changing behaviour. Most of this is a rename: socket.error and select.error are aliases of OSError in Python 3 - literally the same object - so spelling them OSError says the same thing in the spelling the language uses. Alongside that, `except X as e` where e is never referenced loses the binding, three `pass` statements trailing an if inside an except body are dropped as dead, and a handful of handlers are narrowed to what the guarded call can actually raise: - auto_mavlink_version()'s ord(buf[0]) raises TypeError when buf is bytes and buf[0] is therefore already an int; that is the whole reason for the fallback. - set_rtscts()/set_baudrate() fall back to attribute assignment because pySerial 3.0 dropped setRtsCts()/setBaudrate(); a missing method is AttributeError, and the comment already says so. - getsockname(), connect() and accept() raise OSError. - the curses import can only fail with ImportError. - the custom mode map's dict-building step raises ValueError from int(), or AttributeError/TypeError when the JSON is not shaped like a mapping. Its inner `except OSError: raise` is deleted: re-raising immediately is what not catching does, and the outer handler already covers it. Deliberately left broad, having been flagged in review as regressions when narrowed: - the optional top-level dialect import and set_dialect()'s regenerate fallback. Both are best-effort by intent - the latter self-heals a corrupt or partially written generated dialect, which fails with SyntaxError rather than ImportError. - set_close_on_exec(), whose docstring is "Ignore exceptions". fcntl raises TypeError for fd=None and ValueError for a negative fd, neither an OSError. - both fileno() probes and mavserial.write()/reset(). pySerial's win32 backend does not define fileno() and so inherits io.RawIOBase.fileno(), which raises io.UnsupportedOperation - an OSError and ValueError, but not a SerialException. FakeSerial.write() raises a bare Exception on purpose, so narrowing write() breaks the force_connected path, and reset() promises a bool rather than raising. Also note serial.SerialException already subclasses OSError, so (serial.SerialException, OSError) would have collapsed to OSError anyway. - auto_detect_serial_win32(), which must degrade to an empty list whatever pySerial's enumeration does. Verified with mypy and the test suite; 33 passed, 8 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…achable connect() listed its socket error handler before its ssl.SSLError handler: except socket.error as e: # socket.error is OSError if e.errno in [errno.ECONNREFUSED, errno.EHOSTUNREACH]: self.close() return raise except ssl.SSLError as e: print(f"SSL Error: {e}") self.close() raise ssl.SSLError subclasses OSError, so the first clause always won and the second was dead code. An SSL failure fell into the errno test, matched neither ECONNREFUSED nor EHOSTUNREACH, and reached the bare raise - propagating without the diagnostic and, more importantly, without closing the socket, leaking it on every failed handshake. Order the specific handler first. A failed handshake now prints and closes before re-raising, which is what the code was written to do. The other three try blocks in this class already have ssl.SSLError first and are unaffected. Their ordering is what makes the bug in connect() easy to miss. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

khancyr · 2026-08-07T15:07:20Z

@peterbarker I tried to keep the one's that shouldn't be blocking. Other may need so other change so part of another PR

peterbarker

I don't think I will be able to approve/merge these, I'm afraid, I think tridge will need to do that.

We would probably need to do an audit of MAVProxy to make sure that its resilience is not impacted by this. Changing the API from "catch exception, return value" to what it should have been (if you can't handle an exception then don't look for the problem) is not a small change to be making!

peterbarker · 2026-08-10T01:05:47Z

    AP_MAV_TYPE_MODE_MAP = AP_MAV_TYPE_MODE_MAP_DEFAULT.copy()
    AP_MAV_TYPE_MODE_MAP.update(_custom_mode_map)
-except Exception:
+except (OSError, ValueError, AttributeError, TypeError, json.decoder.JSONDecodeError):


Please revert this one. You're missing at least RecursionError

I'd actually like to consider removing the entire try/except block here on the basis that the mode mapping file is relatively static and the user probably does want an early-exit with mavutil.py is imported!

peterbarker · 2026-08-10T01:09:10Z

        try:
            self.port.setRtsCts(enable)
-        except Exception:
+        except AttributeError:


Suggested change

except AttributeError:

except (AttributeError, NotImplementedError):

khancyr commented Aug 6, 2026

View reviewed changes

khancyr and others added 2 commits August 7, 2026 16:58

khancyr force-pushed the tidy-exceptions branch from cb04a95 to ce49f2e Compare August 7, 2026 15:06

peterbarker added this to Peter's ArduPilot 4.8 Queue Aug 9, 2026

peterbarker requested changes Aug 10, 2026

View reviewed changes

Navigation Menu

Uh oh!

mavutil: tidy exceptions - #1252

mavutil: tidy exceptions#1252
khancyr wants to merge 2 commits into
ArduPilot:masterfrom
khancyr:tidy-exceptions

khancyr commented Aug 6, 2026

Uh oh!

khancyr Aug 6, 2026

Uh oh!

peterbarker commented Aug 7, 2026

Uh oh!

khancyr commented Aug 7, 2026

Uh oh!

peterbarker left a comment

Uh oh!

peterbarker Aug 10, 2026

Uh oh!

peterbarker Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	except AttributeError:
	except (AttributeError, NotImplementedError):

Uh oh!

Conversation

khancyr commented Aug 6, 2026

Uh oh!

khancyr Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

peterbarker commented Aug 7, 2026

Uh oh!

khancyr commented Aug 7, 2026

Uh oh!

peterbarker left a comment

Choose a reason for hiding this comment

Uh oh!

peterbarker Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

peterbarker Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants