Merge pull request #1283 from girder/fix-bad-style-handles

Closing some styled sources could premature close file handles

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

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

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)
 

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:

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
 

test/test_cache_source.py

@@ -1,3 +1,5 @@
+import sys
+
 import pytest
 
 import large_image
@@ -45,3 +47,21 @@ 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.skipif(sys.version_info < (3, 7),
+                    reason='requires python >= 3.7 for a source with the issue')
+@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()