Skip to content

Release partial mmaps when MemoryMappedDataset init fails#56

Merged
mmshad merged 2 commits into
mainfrom
mmap-init-failure-cleanup
Apr 22, 2026
Merged

Release partial mmaps when MemoryMappedDataset init fails#56
mmshad merged 2 commits into
mainfrom
mmap-init-failure-cleanup

Conversation

@mmshad
Copy link
Copy Markdown
Collaborator

@mmshad mmshad commented Apr 22, 2026

Summary

  • Wrap MemoryMappedDataset.__init__'s file-open loop in try/except that calls _close_mmaps() on failure and re-raises, so partial state does not leak through the exception traceback (pytest frames, logger.exception, post-mortem debuggers).
  • Add close() for explicit release and __del__ for GC fallback.
  • _close_mmaps suppresses BufferError (live views into the mapping) and ValueError (already closed by another path) so cleanup is always idempotent.

Closes #55

Test plan

  • uv run pytest tests/unit/test_dataset_mmap_cleanup.py -v (4 tests).
  • Full unit suite clean.

MemoryMappedDataset.__init__ opens one mmap per file in a loop and
appends to self._mmaps. If any open raised partway through, the prior
opens stayed reachable through the exception traceback (pytest frames,
logger.exception, post-mortem debuggers), leaving their virtual-memory
mappings pinned for the lifetime of the traceback. Under retry loops or
long-lived test sessions on Lustre/NFS, this accumulates.

Wrap the open loop in try/except that calls _close_mmaps() on failure
and re-raises. Add close() for explicit release and __del__ for GC
fallback. _close_mmaps uses contextlib.suppress(BufferError, ValueError)
because an inner close can fail when views into the mapping are still
live or the mmap was already closed by another path.
@mmshad mmshad self-assigned this Apr 22, 2026
@mmshad mmshad requested a review from Naeemkh April 22, 2026 01:12
Copy link
Copy Markdown
Member

@Naeemkh Naeemkh left a comment

Choose a reason for hiding this comment

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

Relying on del for cleanup is generally discouraged (interpreter shutdown ordering, reference cycles, etc.). The contextlib.suppress(Exception) is the right defensive move, but consider documenting that close() is the preferred path and __del__ is a safety net only.

@mmshad
Copy link
Copy Markdown
Collaborator Author

mmshad commented Apr 22, 2026

Relying on del for cleanup is generally discouraged (interpreter shutdown ordering, reference cycles, etc.). The contextlib.suppress(Exception) is the right defensive move, but consider documenting that close() is the preferred path and __del__ is a safety net only.

Good catch! Pushing a follow-up on this branch: docstring on __del__ flagging it as a GC safety net, and a tighter close() docstring naming the explicit path as preferred. Happy to also add __enter__/__exit__ for a context-manager path if you want it. Let me know.

close() is the explicit API; __del__ is a GC safety net. Empty __del__
docstring previously left the intent implicit. Per review feedback.
@mmshad mmshad merged commit b5dde50 into main Apr 22, 2026
5 checks passed
@mmshad mmshad deleted the mmap-init-failure-cleanup branch April 22, 2026 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MemoryMappedDataset leaks partial mmaps when init fails

2 participants