Merge pull request #753 from effigies/fix/warning_filters

MRG: Safer warning registry manipulation when checking for overflows

This approach unconditionally prepends the filter, which hopefully reduces the odds that removing it at the end will interfere with any other threads attempts to modify the registry. In particular, it should be re-entrant, so multiple threads calling this same function should not interfere with each other unless they happen to race on modifying the list. We could add a lock around the insertion/removal to avoid self-interference, though as noted in the other thread, no other libraries will respect it.

Looking at Python 3.4+, adding filters to the filter list is followed by a call to `warnings._filters_mutated()`, so I'm adding that behavior in, even though it's not part of the API. When Python 2 is dropped, the `getattr` part can be removed, though perhaps it's safer to do this for private functions anyway.

Closes #616

Author: matthew-brett

nibabel/tests/test_volumeutils.py

@@ -19,6 +19,8 @@
 import itertools
 import gzip
 import bz2
+import threading
+import time
 
 import numpy as np
 
@@ -45,6 +47,7 @@
                            rec2dict,
                            _dt_min_max,
                            _write_data,
+                           _ftype4scaled_finite,
                            )
 from ..openers import Opener, BZ2File
 from ..casting import (floor_log2, type_info, OK_FLOATS, shared_range)
@@ -1245,3 +1248,50 @@ def read(self, n_bytes):
                  'Expected {0} bytes, got {1} bytes from {2}\n'
                  ' - could the file be damaged?'.format(
                      11390625000000000000, 0, 'object'))
+
+
+def test__ftype4scaled_finite_warningfilters():
+    # This test checks our ability to properly manage the thread-unsafe
+    # warnings filter list.
+
+    # _ftype4scaled_finite always operates on one-or-two element arrays
+    # Ensure that an overflow will happen for < float64
+    finfo = np.finfo(np.float32)
+    tst_arr = np.array((finfo.min, finfo.max), dtype=np.float32)
+
+    go = threading.Event()
+    stop = threading.Event()
+    err = []
+    class MakeTotalDestroy(threading.Thread):
+        def run(self):
+            # Restore the warnings filters when we're done testing
+            with warnings.catch_warnings():
+                go.set()
+                while not stop.is_set():
+                    warnings.filters[:] = []
+                    time.sleep(0)
+    class CheckScaling(threading.Thread):
+        def run(self):
+            go.wait()
+            # Give ourselves a few bites at the apple
+            # 200 loops through the function takes ~10ms
+            # The highest number of iterations I've seen before hitting interference
+            # is 131, with 99% under 30, so this should be reasonably reliable.
+            for i in range(200):
+                try:
+                    # Use float16 to ensure two failures and increase time in function
+                    _ftype4scaled_finite(tst_arr, 2.0, 1.0, default=np.float16)
+                except Exception as e:
+                    err.append(e)
+                    break
+            stop.set()
+
+    thread_a = CheckScaling()
+    thread_b = MakeTotalDestroy()
+    thread_a.start()
+    thread_b.start()
+    thread_a.join()
+    thread_b.join()
+
+    if err:
+        raise err[0]

nibabel/volumeutils.py

@@ -1334,26 +1334,30 @@ def _ftype4scaled_finite(tst_arr, slope, inter, direction='read',
     tst_arr = np.atleast_1d(tst_arr)
     slope = np.atleast_1d(slope)
     inter = np.atleast_1d(inter)
-    warnings.filterwarnings('ignore', '.*overflow.*', RuntimeWarning)
-    try:
-        for ftype in OK_FLOATS[def_ind:]:
-            tst_trans = tst_arr.copy()
-            slope = slope.astype(ftype)
-            inter = inter.astype(ftype)
-            if direction == 'read':  # as in reading of image from disk
-                if slope != 1.0:
-                    tst_trans = tst_trans * slope
-                if inter != 0.0:
-                    tst_trans = tst_trans + inter
-            elif direction == 'write':
-                if inter != 0.0:
-                    tst_trans = tst_trans - inter
-                if slope != 1.0:
-                    tst_trans = tst_trans / slope
+    overflow_filter = ('error', '.*overflow.*', RuntimeWarning)
+    for ftype in OK_FLOATS[def_ind:]:
+        tst_trans = tst_arr.copy()
+        slope = slope.astype(ftype)
+        inter = inter.astype(ftype)
+        try:
+            with warnings.catch_warnings():
+                # Error on overflows to short circuit the logic
+                warnings.filterwarnings(*overflow_filter)
+                if direction == 'read':  # as in reading of image from disk
+                    if slope != 1.0:
+                        tst_trans = tst_trans * slope
+                    if inter != 0.0:
+                        tst_trans = tst_trans + inter
+                elif direction == 'write':
+                    if inter != 0.0:
+                        tst_trans = tst_trans - inter
+                    if slope != 1.0:
+                        tst_trans = tst_trans / slope
+            # Double-check that result is finite
             if np.all(np.isfinite(tst_trans)):
                 return ftype
-    finally:
-        warnings.filters.pop(0)
+        except RuntimeWarning:
+            pass
     raise ValueError('Overflow using highest floating point type')