From 68adeaba63290c5847b518b4f38f86f2d13f8530 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Thu, 24 Jul 2025 14:36:42 +0200 Subject: [PATCH 1/3] Harden against memory leak The extensions implementation of packstream `Structure` could leak memory when being part of a reference cycle. In reality this doesn't matter because the driver never constructs cyclic `Structure`s. Every packstream value is a tree in terms of references (both directions: packing and unpacking). This change is meant to harden the extensions against introducing effective memory leaks in the driver should the driver's usage of `Structure` change in the future. --- src/codec/packstream.rs | 13 +++++- tests/codec/packstream/test_structure.py | 53 ++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 tests/codec/packstream/test_structure.py diff --git a/src/codec/packstream.rs b/src/codec/packstream.rs index bcfe97d..55a2e36 100644 --- a/src/codec/packstream.rs +++ b/src/codec/packstream.rs @@ -19,7 +19,7 @@ use pyo3::basic::CompareOp; use pyo3::exceptions::PyValueError; use pyo3::prelude::*; use pyo3::types::{PyBytes, PyTuple}; -use pyo3::IntoPyObjectExt; +use pyo3::{IntoPyObjectExt, PyTraverseError, PyVisit}; use crate::register_package; @@ -101,4 +101,15 @@ impl Structure { } Ok(fields_hash.wrapping_add(self.tag.into())) } + + fn __traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError> { + for field in &self.fields { + visit.call(field)?; + } + Ok(()) + } + + fn __clear__(&mut self) { + self.fields.clear(); + } } diff --git a/tests/codec/packstream/test_structure.py b/tests/codec/packstream/test_structure.py new file mode 100644 index 0000000..05bf110 --- /dev/null +++ b/tests/codec/packstream/test_structure.py @@ -0,0 +1,53 @@ +# Copyright (c) "Neo4j" +# Neo4j Sweden AB [https://neo4j.com] +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + + +import gc +from contextlib import contextmanager + +from neo4j._codec.packstream import Structure + + +@contextmanager +def gc_disabled(): + try: + gc.disable() + yield + finally: + gc.enable() + gc.collect() + + +class StructureHolder: + s: Structure | None = None + + +def test_memory_leak() -> None: + iterations = 10_000 + + gc.collect() + with gc_disabled(): + for _ in range(iterations): + # create a reference cycle + holder1 = StructureHolder() + structure1 = Structure(b"\x00", [holder1]) + holder2 = StructureHolder() + structure2 = Structure(b"\x01", [holder2]) + holder1.s = structure2 + holder2.s = structure1 + del structure1, structure2, holder1, holder2 + + cleaned = gc.collect() + assert cleaned >= 4 * iterations From baaa8564975719c08448b6a8bf4122cc3e651951 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Fri, 25 Jul 2025 09:15:17 +0200 Subject: [PATCH 2/3] Add changelog entry --- changelog.d/50.improve.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog.d/50.improve.md diff --git a/changelog.d/50.improve.md b/changelog.d/50.improve.md new file mode 100644 index 0000000..1660ee8 --- /dev/null +++ b/changelog.d/50.improve.md @@ -0,0 +1,5 @@ +Harden `Structure` class against memory leak. +The extensions' implementation of packstream `Structure` could leak memory when being part of a reference cycle. +In reality this doesn't matter because the driver never constructs cyclic `Structure`s. +Every packstream value is a tree in terms of references (both directions: packing and unpacking). +This change is meant to harden the extensions against introducing effective memory leaks in the driver should the driver's usage of `Structure` change in the future. From 6e8a7d8320e19d802ac29d56f28dcecb1236b079 Mon Sep 17 00:00:00 2001 From: Robsdedude Date: Tue, 29 Jul 2025 14:34:56 +0200 Subject: [PATCH 3/3] Minor clean-up --- tests/codec/packstream/test_structure.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/codec/packstream/test_structure.py b/tests/codec/packstream/test_structure.py index 05bf110..1a25e6e 100644 --- a/tests/codec/packstream/test_structure.py +++ b/tests/codec/packstream/test_structure.py @@ -14,6 +14,8 @@ # limitations under the License. +from __future__ import annotations + import gc from contextlib import contextmanager