Allow memmapped arrays to keep the backing mmap file open #1668
Conversation
braingram
changed the title
braingram
force-pushed
the
memmap_open
branch
3 times, most recently
from
303c6c9
to
3fa2ea2
Compare
| if hasattr(self, "_mmap") and not self._mmap.closed: | ||
| self._mmap.close() | ||
| if hasattr(self, "_mmap"): | ||
| # we no longer close the _mmap here. This does mean that views of arrays |
There was a problem hiding this comment.
I think we got the better end of that trade.
| if fd is not None: | ||
| if getattr(fd, "_mmap", None) is not base.base: | ||
| self._array = None | ||
| del fd |
There was a problem hiding this comment.
Just a question for my own educational purposes, what does explicitly deleting the fd variable accomplish?
There was a problem hiding this comment.
Good question. The _fd attr is a weakef so I think I added this mostly out of habit. The explicit del of fd isn't needed here as fd isn't used or stored (so the reference will be released at the end of the function).
|
Thanks for the reviews! |
|
|
braingram
force-pushed
the
memmap_open
branch
from
3fa2ea2
to
48da84c
Compare
|
Roman regtests ran with no errors:
I re-ran the test_duplicate_names test and it passed with this PR: https://plwishmaster.stsci.edu:8081/blue/organizations/jenkins/RT%2FJWST-Developers-Pull-Requests/detail/JWST-Developers-Pull-Requests/1094/pipeline/199 At this point I'm claiming the regtests all 'pass' with this PR and any failures are unrelated. |
asdf uses a mmap to memory map files to produce np.memmap arrays (when copy_arrays is True, the current default). np.memmap does not check if the backing mmap is closed before accessing array data and asdf closes the mmap when the asdf file is closed. This results in segfaults (see issue 1334). This commit changes when happens to the mmap when the asdf file is closed. asdf will no longer force closure of the mmap. This will result in an open file pointer if any array still references the mmap (the mmap will likely be closed by the garbage collector when all arrays are collected which matches the behavior of np.memmap).
braingram
force-pushed
the
memmap_open
branch
from
48da84c
to
0c0aef4
Compare
asdf uses a mmap to memory map files to produce np.memmap arrays (when copy_arrays is True, the current default). np.memmap does not check if the backing mmap is closed before accessing array data and asdf closes the mmap when the asdf file is closed. This results in segfaults (see issue #1334).
This PR changes when happens to the mmap when the asdf file is closed. asdf will no longer force closure of the mmap. This will result in an open file pointer if any array still references the mmap (the mmap will likely be closed by the garbage collector when all arrays are collected which matches the behavior of np.memmap which does not have an API for closing the underlying mmap).
Windows does not allow a file to be trunacated (as can occur during
update) if it is open more than once. The PR adds an os check and closes the mmap on windows which (with these changes) passes all tests:https://github.com/asdf-format/asdf/actions/runs/6657397904/job/18092170804?pr=1668
including the regression test added with this PR that segfaults on linux and mac.
The failure of the
buildjob is unrelated and addressed in: #1670The python 3.12 failure is unrelated (due to
aiohttphaving no released version that supports 3.12).The stdatamodels failure should be addressed prior to this PR being merged. The failures are all related to the change in the number of open files (which stdatamodels has specific tests for). spacetelescope/stdatamodels#232 addresses these tests.EDIT: spacetelescope/stdatamodels#232 is mergedFixes #1334
Fixes #1530