Fix: get_data_as_view retaion the backing reader/builder even after the view is released, leading to memory leakage - #406
Merged
Conversation
Use a module-level sentinel when Cap'n Proto reports a NULL pointer with zero size so PyBuffer_FillInfo receives a valid address for unset fields. Co-authored-by: Cursor <cursoragent@cursor.com>
PyBuffer_FillInfo pins `self` via buf.obj; call PyBuffer_Release on failure so that reference is not leaked. This is safe for sentinel-backed empty views: PyBuffer_Release only decrements buf.obj and does not free buf.buf. Co-authored-by: Cursor <cursoragent@cursor.com>
Document borrowing semantics, mutation hazards, and empty DATA field behavior for get_data_as_view and to_segment_views. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace PyMemoryView_FromBuffer with a _BorrowedBufferView holder and PyMemoryView_FromObject so get_data_as_view() correctly pins the struct reader/builder for the memoryview lifetime. Generalize the same exporter for to_segment_views() and add regression tests for packed payload release. Co-authored-by: Cursor <cursoragent@cursor.com>
get_data_as_view retaion the backing reader/builder even after the view is released, leading to memory leakage
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
pycapnp
get_data_as_view()Memory Retention Issueget_data_as_view()appears to retain the backing reader/builder after the memoryview is releasedEnvironment
2.2.3(original report); re-verified on2.2.43.11(original report); re-verified on3.10(venv)Summary
Calling
get_data_as_view()on aDatafield seems to keep the backing Cap'n Proto reader/builder alive even after:memoryviewis deletedmemoryview.release()is calledgc.collect()andmalloc_trim(0)are calledFor packed readers, this also keeps the original packed input buffer alive. This causes linear RSS growth when decoding messages with large
Datafields in a loop.Verification status: confirmed. The reproducer below was re-run locally and produced identical numbers to the original report.
Fix status: implemented in commit
ded8d02(fix: pin DATA field views via shared buffer exporter). See Implementation and Testing.Expected Behavior
Intended lifetime semantics
The desired contract for the returned
memoryview(mv) is:mvis alive (Python refcount > 0): the underlying buffer must remain valid and must not be freed, even if the user deletes their Python variable for the reader/builder.mvis gone (del mv, or refcount reaches 0 aftermv.release()): the underlying buffer should become immediately collectible, even if reader/builder no longer exist in Python.In other words,
mvshould pin the backing storage for the duration of its own lifetime, and release that pin whenmvitself is destroyed.Naive reference chain
A natural mental model is:
For packed readers the full chain is:
Actual Behavior
The backing objects remain alive after
view.release()anddel view.Observed with a 4 MiB
Datafield:Re-verification on pycapnp 2.2.4 (Python 3.10, WSL2):
from_bytes_packed()alone does not leak. The leak appears only after callingreader.get_data_as_view("data").Minimal Reproducer
Analysis
Buggy implementation (before fix)
In
capnp/lib/capnp.pyx, both reader and builderget_data_as_view()follow the same pattern:The comments in the source correctly note that
PyBuffer_FillInfotakes a reference toselfviabuf.obj, and thatPyBuffer_Release(&buf)is called only on the exception path.Root cause: leaked
Py_bufferreferencePer the Python C API:
The call sequence inside
get_data_as_view()is:PyBuffer_FillInfo(&buf, self, ptr, len, ...)—INCREF(self)viabuf.objPyMemoryView_FromBuffer(&buf)— copies the raw pointer into a newmemoryviewobjectPyBuffer_Release(&buf)on the success pathWithout step 3, each call permanently leaks one reference to the reader/builder. After
del view, reader, that leaked reference keeps the entire chain alive (reader →_PackedMessageReaderBytes→ packed input buffer).This explains:
+8 MiB/iterfor packed readers (~4 MiB payload + ~4 MiB decoded message)+4 MiB/iterfor builders (message segment only)weakref payloadstill alive afterview.release()anddel view, readerPyMemoryView_FromBufferdoes NOT pin the exporterCPython 3.11 source (
Objects/memoryobject.c) makes an important distinction:Key points:
PyMemoryView_FromBuffer()copies the raw pointer but forcesmaster.obj = NULLmemoryviewtherefore hasmv.obj is Noneand does not hold a Python reference to the reader/builderPyMemoryView_GET_BASE()also returnsNULLfor views created this waySo the naive chain
mv ──ref──> readerdoes not exist in the current implementation. Thememoryviewis a bare pointer view, not an exporter-backed view.Additionally, the buffer protocol docs state that when
PyBuffer_FillInfois not used inside agetbufferproc, theexporterargument must beNULL. The current code passesselfas exporter outside ofgetbufferproc, which is outside the documented contract.Reference counts (buggy vs fixed)
Assume a single Python variable
readerpointing at the struct reader.mvrefcountreaderdelta vs baselineget_data_as_view()(buggy)PyMemoryView_FromBufferbare pointer; localbufnever releasedget_data_as_view()(fixed)mv.objholds exporter; exporter._owner holds readerdel mv(buggy)del mv(fixed)Measured on pycapnp 2.2.4 (Python 3.10):
After the fix, the
+1whilemvis alive is intentional (exporter pins reader). Afterdel mv, the pin is released and memory becomes collectable.Lifetime semantics experiments
Additional tests were run to check the two intended lifetime rules.
Case 1:
del reader, keepmvmvusable afterdel reader?mvalive?After the fix, this works by design:
mv → exporter → readerkeeps the reader object alive even when the user'sreadervariable is deleted.Case 2:
del reader, thendel mv— should release bufCase 3: loop 20× (
del readerthendel mveach iteration)Implementation
Commit:
fix: pin DATA field views via shared buffer exporterThe fix generalizes the existing
_SegmentViewinfrastructure so thatget_data_as_view()andto_segment_views()share one buffer-protocol exporter class. The oldPyBuffer_FillInfo+PyMemoryView_FromBufferpath is removed entirely fromget_data_as_view().Design
The
_SegmentViewhelper incapnp/lib/capnp.pyxalready implemented the correct buffer-protocol pattern:Both APIs now share infrastructure but keep different public return shapes:
to_segment_views()_MessageBuilderget_data_as_view()(reader)_DynamicStructReadermemoryviewget_data_as_view()(builder)_DynamicStructBuildermemoryview_BorrowedBufferView_SegmentViewwas generalized to_BorrowedBufferView: a generic exporter with_owner,_ptr,_size, and_readonlyfields, implementing__getbuffer__/__releasebuffer__._SegmentViewsnow creates_BorrowedBufferView(..., readonly=True)instances instead of_SegmentView. No public API change forto_segment_views()._memoryview_borrowinghelperThis establishes the reference chain:
mv.objnow points at the exporter object (notNone), and lifetime is enforced by standard buffer-protocol reference counting.get_data_as_view()on reader and builderDATA field pointer resolution is factored into
_data_field_ptr_readerand_data_field_ptr_builder. Bothget_data_as_view()methods resolve the field pointer then call_memoryview_borrowing:Empty-field handling is unchanged (
data_size == 0 and data_ptr == NULL→_EMPTY_DATA_VIEW_SENTINEL).Owner pinning: struct vs message
to_segment_views()exports whole-message segment snapshots → pins_MessageBuilder.get_data_as_view()exports a pointer inside a specific struct's DATA field → pinsself(the struct reader/builder):API compatibility
get_data_as_view()still returnsmemoryviewdirectly — no breaking change for callers.to_segment_views()still returns a sequence of exporter objects; callers wrap withmemoryview()as before.Files changed
capnp/lib/capnp.pyx_BorrowedBufferView,_memoryview_borrowing,_data_field_ptr_*, refactor_SegmentViews, rewrite bothget_data_as_view()test/test_get_data_view.pyStructure
Testing
New regression tests (
test/test_get_data_view.py)test_data_view_exports_through_buffer_exporterview.obj is not None; exporter length matches viewtest_data_view_survives_del_builderdel msgwhile view alive → view still readabletest_data_view_releases_packed_payloadview.release()+del view, reader, payload→ weakref payload isNoneExisting tests continue to cover read-only/writable behavior, empty DATA fields, nested structs, wrong field types, and
test_view_keeps_message_alive(refcount increase via exporter pin).Targeted test runs
python -m pytest test/test_get_data_view.py test/test_serialization.py -q # 36 passedAll segment-view tests pass unchanged, confirming
_BorrowedBufferViewgeneralization did not regressto_segment_views()behavior.Full test suite
Run on pycapnp 2.2.4, Python 3.10, WSL2 (with async test deps installed):
The single failure is unrelated to this fix:
test/test_load.py::test_bundled_import_hook—ImportError: cannot import name 'stream_capnp' from 'capnp'. This is a dev-environment issue: bundled.capnpschemas are not on the import path under editable installs. CI/tox runspip install .first and does not hit this.Reproducer after fix
Re-running the RSS reproducer from this document on the fixed build:
Notes
In the buggy build, the returned
memoryviewreportedobj is None, and explicitmemoryview.release()did not release the backing owner. The apparent "pinning whilemvis alive" was largely a side effect of the leakedPyBuffer_FillInforeference, not a correct exporter-backed buffer view.After the fix,
mv.objexposes the internal_BorrowedBufferViewexporter, and lifetime follows standard Python buffer-protocol semantics: the view pins the struct reader/builder (and thus the message/payload) while alive, and releases it when the view is destroyed.