From 63a3f51bf902966d1ac5493218e5191b3968817e Mon Sep 17 00:00:00 2001 From: Shai <66793913+Shaisolaris@users.noreply.github.com> Date: Tue, 20 Oct 2026 06:27:02 -0700 Subject: [PATCH 1/3] Close mmap files when a MmapedDict is discarded Dropping the object without close() left the backing file open, so CPython reported an unclosed-file ResourceWarning. Growing the map also replaced the previous mmap without closing it. Close the previous map before a larger map is created, and close both the map and the file from close() and __del__. Signed-off-by: Shai <66793913+Shaisolaris@users.noreply.github.com> --- prometheus_client/mmap_dict.py | 41 ++++++++++------------------------ 1 file changed, 12 insertions(+), 29 deletions(-) diff --git a/prometheus_client/mmap_dict.py b/prometheus_client/mmap_dict.py index edd895cd..090ac911 100644 --- a/prometheus_client/mmap_dict.py +++ b/prometheus_client/mmap_dict.py @@ -11,11 +11,6 @@ _unpack_two_doubles = struct.Struct(b'dd').unpack_from -# struct.pack_into has atomicity issues because it will temporarily write 0 into -# the mmap, resulting in false reads to 0 when experiencing a lot of writes. -# Using direct assignment solves this issue. - - def _pack_two_doubles(data, pos, value, timestamp): data[pos:pos + 16] = _pack_two_doubles_func(value, timestamp) @@ -28,14 +23,12 @@ def _read_all_values(data, used=0): """Yield (key, value, timestamp, pos). No locking is performed.""" if used <= 0: - # If not valid `used` value is passed in, read it from the file. used = _unpack_integer(data, 0)[0] pos = 8 while pos < used: encoded_len = _unpack_integer(data, pos)[0] - # check we are not reading beyond bounds if encoded_len + pos > used: raise RuntimeError('Read beyond file size detected, file is corrupted.') pos += 4 @@ -48,17 +41,7 @@ def _read_all_values(data, used=0): class MmapedDict: - """A dict of doubles, backed by an mmapped file. - - The file starts with a 4 byte int, indicating how much of it is used. - Then 4 bytes of padding. - There's then a number of entries, consisting of a 4 byte int which is the - size of the next field, a utf-8 encoded string key, padding to a 8 byte - alignment, and then a 8 byte float which is the value and a 8 byte float - which is a UNIX timestamp in seconds. - - Not thread safe. - """ + """A dict of doubles, backed by an mmapped file.""" def __init__(self, filename, read_mode=False): self._f = open(filename, 'rb' if read_mode else 'a+b') @@ -84,37 +67,32 @@ def __init__(self, filename, read_mode=False): @staticmethod def read_all_values_from_file(filename): with open(filename, 'rb') as infp: - # Read the first block of data, including the first 4 bytes which tell us - # how much of the file (which is preallocated to _INITIAL_MMAP_SIZE bytes) is occupied. data = infp.read(mmap.PAGESIZE) used = _unpack_integer(data, 0)[0] - if used > len(data): # Then read in the rest, if needed. + if used > len(data): data += infp.read(used - len(data)) return _read_all_values(data, used) def _init_value(self, key): """Initialize a value. Lock must be held by caller.""" encoded = key.encode('utf-8') - # Pad to be 8-byte aligned. padded = encoded + (b' ' * (8 - (len(encoded) + 4) % 8)) value = struct.pack(f'i{len(padded)}sdd'.encode(), len(encoded), padded, 0.0, 0.0) while self._used + len(value) > self._capacity: self._capacity *= 2 self._f.truncate(self._capacity) - self._m = mmap.mmap(self._f.fileno(), self._capacity) + self._m.close() + self._m = mmap.mmap(self._f.fileno(), self._capacity, access=mmap.ACCESS_WRITE) self._m[self._used:self._used + len(value)] = value - # Update how much space we've used. self._used += len(value) _pack_integer(self._m, 0, self._used) self._positions[key] = self._used - 16 def _read_all_values(self): - """Yield (key, value, pos). No locking is performed.""" return _read_all_values(data=self._m, used=self._used) def read_all_values(self): - """Yield (key, value, timestamp). No locking is performed.""" for k, v, ts, _ in self._read_all_values(): yield k, v, ts @@ -131,15 +109,20 @@ def write_value(self, key, value, timestamp): _pack_two_doubles(self._m, pos, value, timestamp) def close(self): - if self._f: + if getattr(self, "_m", None) is not None: self._m.close() self._m = None + if getattr(self, "_f", None) is not None: self._f.close() self._f = None + def __del__(self): + try: + self.close() + except Exception: + pass + def mmap_key(metric_name: str, name: str, labelnames: List[str], labelvalues: List[str], help_text: str) -> str: - """Format a key for use in the mmap file.""" - # ensure labels are in consistent order for identity labels = dict(zip(labelnames, labelvalues)) return json.dumps([metric_name, name, labels, help_text], sort_keys=True) From b010fc4a19480a39b9bdb3d6ea7d003ebfd1df3e Mon Sep 17 00:00:00 2001 From: Shai <66793913+Shaisolaris@users.noreply.github.com> Date: Tue, 20 Oct 2026 06:28:40 -0700 Subject: [PATCH 2/3] Add a close test for MmapedDict Cover the unclosed-file warning and values written through a map growth. Signed-off-by: Shai <66793913+Shaisolaris@users.noreply.github.com> --- tests/test_mmap_dict_close.py | 47 +++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 tests/test_mmap_dict_close.py diff --git a/tests/test_mmap_dict_close.py b/tests/test_mmap_dict_close.py new file mode 100644 index 00000000..db5adcbe --- /dev/null +++ b/tests/test_mmap_dict_close.py @@ -0,0 +1,47 @@ +import gc +import os +import tempfile +import unittest +import warnings + +import prometheus_client.mmap_dict as mmap_dict +from prometheus_client.mmap_dict import MmapedDict + + +class TestMmapedDictClose(unittest.TestCase): + def test_discarded_dict_does_not_warn_about_an_open_file(self): + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "metrics.db") + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter("always", ResourceWarning) + store = MmapedDict(path) + store.write_value("one", 1.0, 2.0) + del store + gc.collect() + self.assertEqual( + [w for w in caught if issubclass(w.category, ResourceWarning)], + [], + ) + + def test_growth_keeps_written_values(self): + mmap_dict._INITIAL_MMAP_SIZE = 128 + try: + with tempfile.TemporaryDirectory() as tmp: + path = os.path.join(tmp, "metrics.db") + store = MmapedDict(path) + for i in range(40): + store.write_value("k" * 20 + str(i), float(i), 1.0) + store.close() + values = { + key: value + for key, value, _, _ in MmapedDict.read_all_values_from_file(path) + } + finally: + mmap_dict._INITIAL_MMAP_SIZE = 1 << 16 + self.assertEqual(values["k" * 20 + "0"], 0.0) + self.assertEqual(values["k" * 20 + "39"], 39.0) + self.assertEqual(len(values), 40) + + +if __name__ == "__main__": + unittest.main() From 3f9680f3174caad54e4c89057d970d773d8d8765 Mon Sep 17 00:00:00 2001 From: Shai <66793913+Shaisolaris@users.noreply.github.com> Date: Tue, 20 Oct 2026 06:37:22 -0700 Subject: [PATCH 3/3] Close mmap files when a MmapedDict is discarded Growing the map replaced the previous mmap without closing it. close() now closes the map and the file independently, and __del__ calls close(). Signed-off-by: Shai <66793913+Shaisolaris@users.noreply.github.com> --- prometheus_client/mmap_dict.py | 41 ++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/prometheus_client/mmap_dict.py b/prometheus_client/mmap_dict.py index 090ac911..fb4a19eb 100644 --- a/prometheus_client/mmap_dict.py +++ b/prometheus_client/mmap_dict.py @@ -11,6 +11,11 @@ _unpack_two_doubles = struct.Struct(b'dd').unpack_from +# struct.pack_into has atomicity issues because it will temporarily write 0 into +# the mmap, resulting in false reads to 0 when experiencing a lot of writes. +# Using direct assignment solves this issue. + + def _pack_two_doubles(data, pos, value, timestamp): data[pos:pos + 16] = _pack_two_doubles_func(value, timestamp) @@ -23,12 +28,14 @@ def _read_all_values(data, used=0): """Yield (key, value, timestamp, pos). No locking is performed.""" if used <= 0: + # If not valid `used` value is passed in, read it from the file. used = _unpack_integer(data, 0)[0] pos = 8 while pos < used: encoded_len = _unpack_integer(data, pos)[0] + # check we are not reading beyond bounds if encoded_len + pos > used: raise RuntimeError('Read beyond file size detected, file is corrupted.') pos += 4 @@ -41,7 +48,17 @@ def _read_all_values(data, used=0): class MmapedDict: - """A dict of doubles, backed by an mmapped file.""" + """A dict of doubles, backed by an mmapped file. + + The file starts with a 4 byte int, indicating how much of it is used. + Then 4 bytes of padding. + There's then a number of entries, consisting of a 4 byte int which is the + size of the next field, a utf-8 encoded string key, padding to a 8 byte + alignment, and then a 8 byte float which is the value and a 8 byte float + which is a UNIX timestamp in seconds. + + Not thread safe. + """ def __init__(self, filename, read_mode=False): self._f = open(filename, 'rb' if read_mode else 'a+b') @@ -67,15 +84,18 @@ def __init__(self, filename, read_mode=False): @staticmethod def read_all_values_from_file(filename): with open(filename, 'rb') as infp: + # Read the first block of data, including the first 4 bytes which tell us + # how much of the file (which is preallocated to _INITIAL_MMAP_SIZE bytes) is occupied. data = infp.read(mmap.PAGESIZE) used = _unpack_integer(data, 0)[0] - if used > len(data): + if used > len(data): # Then read in the rest, if needed. data += infp.read(used - len(data)) return _read_all_values(data, used) def _init_value(self, key): """Initialize a value. Lock must be held by caller.""" encoded = key.encode('utf-8') + # Pad to be 8-byte aligned. padded = encoded + (b' ' * (8 - (len(encoded) + 4) % 8)) value = struct.pack(f'i{len(padded)}sdd'.encode(), len(encoded), padded, 0.0, 0.0) while self._used + len(value) > self._capacity: @@ -85,14 +105,17 @@ def _init_value(self, key): self._m = mmap.mmap(self._f.fileno(), self._capacity, access=mmap.ACCESS_WRITE) self._m[self._used:self._used + len(value)] = value + # Update how much space we've used. self._used += len(value) _pack_integer(self._m, 0, self._used) self._positions[key] = self._used - 16 def _read_all_values(self): + """Yield (key, value, pos). No locking is performed.""" return _read_all_values(data=self._m, used=self._used) def read_all_values(self): + """Yield (key, value, timestamp). No locking is performed.""" for k, v, ts, _ in self._read_all_values(): yield k, v, ts @@ -109,20 +132,24 @@ def write_value(self, key, value, timestamp): _pack_two_doubles(self._m, pos, value, timestamp) def close(self): - if getattr(self, "_m", None) is not None: - self._m.close() + m = getattr(self, '_m', None) + if m is not None: + m.close() self._m = None - if getattr(self, "_f", None) is not None: - self._f.close() + f = getattr(self, '_f', None) + if f is not None: + f.close() self._f = None def __del__(self): try: self.close() except Exception: - pass + return def mmap_key(metric_name: str, name: str, labelnames: List[str], labelvalues: List[str], help_text: str) -> str: + """Format a key for use in the mmap file.""" + # ensure labels are in consistent order for identity labels = dict(zip(labelnames, labelvalues)) return json.dumps([metric_name, name, labels, help_text], sort_keys=True)