always close memmaps with the asdf file#1241
always close memmaps with the asdf file#1241WilliamJamieson merged 1 commit intoasdf-format:masterfrom
Conversation
even when reading an already opened file fixes issue asdf-format#1239 update changelog add regression test docstring fix changelog
cb8bc67 to
ac54b61
Compare
| if self._close: | ||
| if hasattr(self, "_mmap") and not self._mmap.closed: | ||
| self._mmap.flush() | ||
| self._mmap.close() |
There was a problem hiding this comment.
Are there any additional implications of this change? Besides fixing the error introduced in astropy.
There was a problem hiding this comment.
None that I've been able to uncover. It does not appear that any other tests aside from the one added in this PR and the ones in astropy (that load a table from asdf) suffer from this issue. With 2.13 memmaps were handled by numpy.memmap which has no api for closing the memmap (as per the docstring here: https://github.com/numpy/numpy/blob/v1.23.0/numpy/core/memmap.py#L40-L43) so it's difficult if not impossible to say we've mimicked the old behavior.
Is there something specific that you had in mind?
There was a problem hiding this comment.
No, it seems like a reasonable change then.
|
The added regression test fails with asdf 2.14 (and the current master branch) with both pytest-openfiles 0.4.0 and 0.5.0 (as it checks for the number of open files directly instead of relying on the plugin). The test passes with this PR. I'm inclined to ignore the minor coverage change (-0.01%) but am happy to attempt to update the PR with more coverage if that's preferred. |
Coverage seems to fluctuate for strange reasons at times. I didn't see any lines drop coverage so its no big deal. |
even when reading an already opened file
fixes issue #1239
update changelog
add regression test docstring