Commit 81b1033

committed

ENH: Copy lock if filehandle is shared, add tests

1 parent 9d5361a commit 81b1033Copy full SHA for 81b1033

File tree

2 files changed

+48

-19

lines changed

nibabel
- arrayproxy.py
- tests
  - test_arrayproxy.py

2 files changed

+48

-19

lines changed

`‎nibabel/arrayproxy.py‎`

Lines changed: 24 additions & 15 deletions

Original file line number	Diff line number	Diff line change
`@@ -58,6 +58,7 @@`
`58`	`58`
`59`	`59`	`if ty.TYPE_CHECKING: # pragma: no cover`
`60`	`60`	`import numpy.typing as npt`
	`61`	`+ from typing_extensions import Self # PY310`
`61`	`62`
`62`	`63`	`# Taken from numpy/__init__.pyi`
`63`	`64`	`_DType = ty.TypeVar('_DType', bound=np.dtype[ty.Any])`
`@@ -212,19 +213,29 @@ def __init__(self, file_like, spec, *, mmap=True, order=None, keep_file_open=Non`
`212`	`213`	`self.order = order`
`213`	`214`	`# Flags to keep track of whether a single ImageOpener is created, and`
`214`	`215`	`# whether a single underlying file handle is created.`
`215`		`- self._keep_file_open, self._persist_opener = self._should_keep_file_open(`
`216`		`- file_like, keep_file_open`
`217`		`- )`
	`216`	`+ self._keep_file_open, self._persist_opener = self._should_keep_file_open(keep_file_open)`
`218`	`217`	`self._lock = RLock()`
`219`	`218`
`220`		`- def copy(self):`
	`219`	`+ def _has_fh(self) -> bool:`
	`220`	`+ """Determine if our file-like is a filehandle or path"""`
	`221`	`+ return hasattr(self.file_like, 'read') and hasattr(self.file_like, 'seek')`
	`222`	`+`
	`223`	`+ def copy(self) -> Self:`
	`224`	`+ """Create a new ArrayProxy for the same file and parameters`
	`225`	`+`
	`226`	`+ If the proxied file is an open file handle, the new ArrayProxy`
	`227`	`+ will share a lock with the old one.`
	`228`	`+ """`
`221`	`229`	`spec = self._shape, self._dtype, self._offset, self._slope, self._inter`
`222`		`- returnArrayProxy(`
	`230`	`+ new=self.__class__(`
`223`	`231`	`self.file_like,`
`224`	`232`	`spec,`
`225`	`233`	`mmap=self._mmap,`
`226`	`234`	`keep_file_open=self._keep_file_open,`
`227`	`235`	`)`
	`236`	`+ if self._has_fh():`
	`237`	`+ new._lock = self._lock`
	`238`	`+ return new`
`228`	`239`
`229`	`240`	`def __del__(self):`
`230`	`241`	"""If this ``ArrayProxy`` was created with ``keep_file_open=True``,
`@@ -245,13 +256,13 @@ def __setstate__(self, state):`
`245`	`256`	`self.__dict__.update(state)`
`246`	`257`	`self._lock = RLock()`
`247`	`258`
`248`		`- def _should_keep_file_open(self, file_like, keep_file_open):`
	`259`	`+ def _should_keep_file_open(self, keep_file_open):`
`249`	`260`	"""Called by ``__init__``.
`250`	`261`
`251`	`262`	This method determines how to manage ``ImageOpener`` instances,
`252`	`263`	`and the underlying file handles - the behaviour depends on:`
`253`	`264`
`254`		- - whether ``file_like`` is an an open file handle, or a path to a
	`265`	+ - whether ``self.file_like`` is an an open file handle, or a path to a
`255`	`266`	``'.gz'`` file, or a path to a non-gzip file.
`256`	`267`	- whether ``indexed_gzip`` is present (see
`257`	`268`	:attr:`.openers.HAVE_INDEXED_GZIP`).
`@@ -270,24 +281,24 @@ def _should_keep_file_open(self, file_like, keep_file_open):`
`270`	`281`	`and closed on each file access.`
`271`	`282`
`272`	`283`	The internal ``_keep_file_open`` flag is only relevant if
`273`		- ``file_like`` is a ``'.gz'`` file, and the ``indexed_gzip`` library is
	`284`	+ ``self.file_like`` is a ``'.gz'`` file, and the ``indexed_gzip`` library is
`274`	`285`	`present.`
`275`	`286`
`276`	`287`	`This method returns the values to be used for the internal`
`277`	`288`	``_persist_opener`` and ``_keep_file_open`` flags; these values are
`278`	`289`	`derived according to the following rules:`
`279`	`290`
`280`		- 1. If ``file_like`` is a file(-like) object, both flags are set to
	`291`	+ 1. If ``self.file_like`` is a file(-like) object, both flags are set to
`281`	`292`	``False``.
`282`	`293`
`283`	`294`	2. If ``keep_file_open`` (as passed to :meth:``__init__``) is
`284`	`295`	``True``, both internal flags are set to ``True``.
`285`	`296`
`286`		- 3. If ``keep_file_open`` is ``False``, but ``file_like`` is not a path
	`297`	+ 3. If ``keep_file_open`` is ``False``, but ``self.file_like`` is not a path
`287`	`298`	to a ``.gz`` file or ``indexed_gzip`` is not present, both flags
`288`	`299`	are set to ``False``.
`289`	`300`
`290`		- 4. If ``keep_file_open`` is ``False``, ``file_like`` is a path to a
	`301`	+ 4. If ``keep_file_open`` is ``False``, ``self.file_like`` is a path to a
`291`	`302`	``.gz`` file, and ``indexed_gzip`` is present, ``_persist_opener``
`292`	`303`	is set to ``True``, and ``_keep_file_open`` is set to ``False``.
`293`	`304`	`In this case, file handle management is delegated to the`
`@@ -296,8 +307,6 @@ def _should_keep_file_open(self, file_like, keep_file_open):`
`296`	`307`	`Parameters`
`297`	`308`	`----------`
`298`	`309`
`299`		`- file_like : object`
`300`		- File-like object or filename, as passed to ``__init__``.
`301`	`310`	`keep_file_open : { True, False }`
`302`	`311`	Flag as passed to ``__init__``.
`303`	`312`
`@@ -320,10 +329,10 @@ def _should_keep_file_open(self, file_like, keep_file_open):`
`320`	`329`	`raise ValueError('keep_file_open must be one of {None, True, False}')`
`321`	`330`
`322`	`331`	`# file_like is a handle - keep_file_open is irrelevant`
`323`		`- if hasattr(file_like, 'read') andhasattr(file_like, 'seek'):`
	`332`	`+ if self._has_fh():`
`324`	`333`	`return False, False`
`325`	`334`	`# if the file is a gzip file, and we have_indexed_gzip,`
`326`		`- have_igzip = openers.HAVE_INDEXED_GZIP and file_like.endswith('.gz')`
	`335`	`+ have_igzip = openers.HAVE_INDEXED_GZIP and self.file_like.endswith('.gz')`
`327`	`336`
`328`	`337`	`persist_opener = keep_file_open or have_igzip`
`329`	`338`	`return keep_file_open, persist_opener`

`‎nibabel/tests/test_arrayproxy.py‎`

Lines changed: 24 additions & 4 deletions

Original file line number	Diff line number	Diff line change
`@@ -554,16 +554,36 @@ def test_keep_file_open_true_false_invalid():`
`554`	`554`	`ArrayProxy(fname, ((10, 10, 10), dtype))`
`555`	`555`
`556`	`556`
	`557`	`+def islock(l):`
	`558`	`+ # isinstance doesn't work on threading.Lock?`
	`559`	`+ return hasattr(l, 'acquire') and hasattr(l, 'release')`
	`560`	`+`
	`561`	`+`
`557`	`562`	`def test_pickle_lock():`
`558`	`563`	`# Test that ArrayProxy can be pickled, and that thread lock is created`
`559`	`564`
`560`		`- def islock(l):`
`561`		`- # isinstance doesn't work on threading.Lock?`
`562`		`- return hasattr(l, 'acquire') and hasattr(l, 'release')`
`563`		`-`
`564`	`565`	`proxy = ArrayProxy('dummyfile', ((10, 10, 10), np.float32))`
`565`	`566`	`assert islock(proxy._lock)`
`566`	`567`	`pickled = pickle.dumps(proxy)`
`567`	`568`	`unpickled = pickle.loads(pickled)`
`568`	`569`	`assert islock(unpickled._lock)`
`569`	`570`	`assert proxy._lock is not unpickled._lock`
	`571`	`+`
	`572`	`+`
	`573`	`+def test_copy():`
	`574`	`+ # Test copying array proxies`
	`575`	`+`
	`576`	`+ # If the file-like is a file name, get a new lock`
	`577`	`+ proxy = ArrayProxy('dummyfile', ((10, 10, 10), np.float32))`
	`578`	`+ assert islock(proxy._lock)`
	`579`	`+ copied = proxy.copy()`
	`580`	`+ assert islock(copied._lock)`
	`581`	`+ assert proxy._lock is not copied._lock`
	`582`	`+`
	`583`	`+ # If an open filehandle, the lock should be shared to`
	`584`	`+ # avoid changing filehandle state in critical sections`
	`585`	`+ proxy = ArrayProxy(BytesIO(), ((10, 10, 10), np.float32))`
	`586`	`+ assert islock(proxy._lock)`
	`587`	`+ copied = proxy.copy()`
	`588`	`+ assert islock(copied._lock)`
	`589`	`+ assert proxy._lock is copied._lock`

0 commit comments

Comments

(0)

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Commit 81b1033

File tree

2 files changed

2 files changed

`‎nibabel/arrayproxy.py‎`

`‎nibabel/tests/test_arrayproxy.py‎`

0 commit comments