Skip to content

DFReader, mavutil: support use as context managers - #1235

Merged
peterbarker merged 3 commits into
ArduPilot:masterfrom
mgwedd:feature/context-manager-support
Jul 31, 2026
Merged

DFReader, mavutil: support use as context managers#1235
peterbarker merged 3 commits into
ArduPilot:masterfrom
mgwedd:feature/context-manager-support

Conversation

@mgwedd

@mgwedd mgwedd commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What

Adds __enter__/__exit__ to the two base classes that already have a
close() contract:

  • DFReader.DFReader — covers DFReader_binary and DFReader_text
  • mavutil.mavfile — covers every connection type and log reader
    returned by mavlink_connection()

so callers can scope a reader or connection with a plain with
statement:

with DFReader_binary(path) as dflog:
    ...
# filehandle and mmap released here, even on exception

Why

DFReader_binary.__init__ opens the log file and mmaps its descriptor,
so each reader pins a filehandle until close() is called. Since there
is no context manager support, callers either remember to call close()
manually, wrap the reader in contextlib.closing, or — most commonly —
just leak the handle. ArduPilot's own autotest suite and Replay checker
did the latter for years (ArduPilot/ardupilot#32533, fixed in
ArduPilot/ardupilot#33839. It had to use contextlib.closing because this class can't be used in a with block).

This PR fixes the issue for all subclasses and downstream consumers.

Testing

New tests/test_context_manager.py (runs in the existing pytest suite,
no new fixtures — reuses tests/test.BIN):

  • DFReader_binary used in a with block: reads a message, then both
    filehandle and data_map report closed on exit.
  • mavlink_connection() on a dataflash log: same via the dispatch path.
  • mavlink_connection('udpout:...'): socket is open inside the block,
    closed (fileno == -1) on exit.

All three pass locally on Python 3.13 (macOS); flake8 clean with the
repo config.

No behavior change for existing callers — close() semantics are
untouched, and __exit__ returns None so exceptions propagate.

mgwedd added 3 commits July 29, 2026 01:15
DFReader_binary and DFReader_text pin an open file and an mmap until
close() is called, but offer no __enter__/__exit__, so callers cannot
use a with-statement and commonly leak the handles (see
ArduPilot/ardupilot#32533).
All mavfile subclasses expose close(), so a with-statement is the
natural way to scope a connection or log reader. mavlink_connection()
callers get this for the DFReader returns via the matching DFReader
change.
@mgwedd
mgwedd force-pushed the feature/context-manager-support branch from f833795 to 0b373b7 Compare July 29, 2026 07:15
@mgwedd
mgwedd marked this pull request as ready for review July 29, 2026 07:15

@khancyr khancyr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM
small changes. exit will call the mavfile override closing method so we are good.
Still work on py3.9
test are simple but OK

@peterbarker peterbarker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@peterbarker
peterbarker merged commit c35e868 into ArduPilot:master Jul 31, 2026
21 checks passed
@peterbarker

Copy link
Copy Markdown
Contributor

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants