From 75397bc8c94197269ef750056844c1937e02df1f Mon Sep 17 00:00:00 2001 From: David Manthey Date: Thu, 31 Aug 2023 11:08:50 -0400 Subject: [PATCH] Closing some styled sources could premature close file handles This could be reproduced by opening an nd2 file, opening another instance of it with a bad style, then trying to open another instance of it with a good style. The file handles would get closed with the bad style, and then subsequent reads of a good style would fail (and even could segfault). --- CHANGELOG.md | 3 +++ large_image/cache_util/cache.py | 5 +++-- large_image/tilesource/base.py | 3 ++- .../dicom/large_image_source_dicom/__init__.py | 5 ++++- sources/nd2/large_image_source_nd2/__init__.py | 5 ++++- test/test_cache_source.py | 16 ++++++++++++++++ 6 files changed, 32 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23784fa76..e90d1760e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ ### Improvements - Allow scheduling histogram computation in Girder ([#1282](../../pull/1282)) +### Bug Fixes +- Closing some styled sources could lead to prematurely closing file handles ([#1283](../../pull/1283)) + ## 1.23.5 ### Improvements diff --git a/large_image/cache_util/cache.py b/large_image/cache_util/cache.py index d28354afd..433f2cff6 100644 --- a/large_image/cache_util/cache.py +++ b/large_image/cache_util/cache.py @@ -208,11 +208,12 @@ def __call__(cls, *args, **kwargs): # noqa - N805 with subresult._sourceLock: result.__dict__ = subresult.__dict__.copy() result._sourceLock = threading.RLock() - result._setStyle(kwargs['style']) result._classkey = key - result._unstyledInstance = subresult # for pickling result._initValues = (args, kwargs.copy()) + result._unstyledInstance = subresult + # Has to be after setting the _unstyledInstance + result._setStyle(kwargs['style']) with cacheLock: cache[key] = result return result diff --git a/large_image/tilesource/base.py b/large_image/tilesource/base.py index ebe7e3064..e8d1e0054 100644 --- a/large_image/tilesource/base.py +++ b/large_image/tilesource/base.py @@ -203,11 +203,12 @@ def _setStyle(self, style): self._jsonstyle = json.dumps(style, sort_keys=True, separators=(',', ':')) else: try: + self._style = None style = json.loads(style) if not isinstance(style, dict): raise TypeError self._style = JSONDict(style) - except TypeError: + except (TypeError, json.decoder.JSONDecodeError): msg = 'Style is not a valid json object.' raise exceptions.TileSourceError(msg) diff --git a/sources/dicom/large_image_source_dicom/__init__.py b/sources/dicom/large_image_source_dicom/__init__.py index bc47999a5..1c0e3bc4c 100644 --- a/sources/dicom/large_image_source_dicom/__init__.py +++ b/sources/dicom/large_image_source_dicom/__init__.py @@ -144,7 +144,10 @@ def __init__(self, path, **kwargs): self._populatedLevels = len(self._dicom.levels) def __del__(self): - if getattr(self, '_dicom', None) is not None: + # If we have an _unstyledInstance attribute, this is not the owner of + # the _docim handle, so we can't close it. Otherwise, we need to close + # it or the _dicom library may prevent shutting down. + if getattr(self, '_dicom', None) is not None and not hasattr(self, '_unstyledInstance'): try: self._dicom.close() finally: diff --git a/sources/nd2/large_image_source_nd2/__init__.py b/sources/nd2/large_image_source_nd2/__init__.py index dea0cca07..ad4178f00 100644 --- a/sources/nd2/large_image_source_nd2/__init__.py +++ b/sources/nd2/large_image_source_nd2/__init__.py @@ -191,7 +191,10 @@ def __init__(self, path, **kwargs): self._tileLock = threading.RLock() def __del__(self): - if hasattr(self, '_nd2'): + # If we have an _unstyledInstance attribute, this is not the owner of + # the _nd2 handle, so we can't close it. Otherwise, we need to close + # it or the nd2 library complains that we didn't explicitly close it. + if hasattr(self, '_nd2') and not hasattr(self, '_unstyledInstance'): self._nd2.close() del self._nd2 diff --git a/test/test_cache_source.py b/test/test_cache_source.py index 54e0882d1..5cfb070be 100644 --- a/test/test_cache_source.py +++ b/test/test_cache_source.py @@ -45,3 +45,19 @@ def testCacheSourceStyleFirst(): assert ts2.getTile(2, 0, 4) is not None ts1 = large_image.open(imagePath) assert ts1.getTile(0, 0, 4) == tile1 + + +@pytest.mark.singular() +def testCacheSourceBadStyle(): + cachesClear() + imagePath = datastore.fetch('ITGA3Hi_export_crop2.nd2') + ts1 = large_image.open(imagePath, style='{"bands": [{"max": 128}]}') + tile1 = ts1.getTile(0, 0, 0) + # With nd2 files, a bad style could cause a future segfault + with pytest.raises(Exception): + large_image.open(imagePath, style='{"bands": [{%,"max": 128}]}') + ts2 = large_image.open(imagePath, style='{"bands": [{"max": 160}]}') + tile2 = ts2.getTile(0, 0, 0) + assert tile1 == tile2 + ts1 = ts2 = None + cachesClear()