FITS I/O, WINDOWS only: files remain open despite the use of "with" statement

In this PR on another package:
discos/srt-single-dish-tools#91
I'm getting a failure in Appveyor:

>           os.unlink(dst)
E           PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: 'C:\\Users\\appveyor\\AppData\\Local\\Temp\\1\\srttools-test-wx27cxl4\\Lib\\site-packages\\dummyfile.fits'

However, dummyfile.fits should not be open. All I/O on that file with astropy.io.fits.open() is through with statements and this call to unlink is in a function called outside of them, so the file should be closed when it gets to that function.
Has anyone gotten the same error?

Can you provide a standalone snippet here to reproduce your problem? Are you using memmap?

@pllim

Hi @pllim I couldn't do it because it doesn't happen in all cases, and I don't understand what is making a specific test fail and not the others. I triple checked that all fits files are closed before overwriting them, and in Linux and Mac it works properly. But Appveyor throws this error.
I was hoping that someone else had had the same error and knew what kind of conditions could produce it

@Alex-Ian-Hamilton

@Alex-Ian-Hamilton any chance you also encounter this on Windows? 🙂

@matteobachetti

@matteobachetti can you please check if you still see this behavior if you force memmap=False? Please see the warning box in http://astropy.readthedocs.io/en/stable/io/fits/index.html#working-with-large-files regarding memmap.

Or could it be that handler cannot be freed fast enough before OS attempts to move the file. In that case, maybe you can put in a time.sleep(x) before force_move_file(...)?

A quick internet search also brought up "make sure you have the correct file permission" and "make sure you run it with admin privilege", and also a mention of a magical task scheduler. These might or might not be relevant.

I haven't ever noticed this, but just send me some code if you want me to test.

@pllim

@pllim I thought that memmap was set to False by default? I will try to force it. Thanks for the suggestion

@Alex-Ian-Hamilton the failure happens when I run unit tests on Appveyor. If you have some time, could you install these tools and run the unit tests?

For your convenience, I attach a series of commands you can just copy-and-paste (assuming you use Anaconda):

conda create -n srttools_test python=3
source activate srttools
conda install astropy scipy numpy matplotlib pyyaml h5py statsmodels numba
git clone https://github.com/matteobachetti/srt-single-dish-tools.git
cd srt-single-dish-tools
git fetch
git checkout mbfits_converter
python setup.py install
python setup.py test

Thanks!

@matteobachetti

@matteobachetti - memmap is True by default (and this should be better documented : #6497)

@saimn

@saimn @pllim (Also related to #7440 by @cdeil) -- yes, I worked around the problem by forcing memmap=False everywhere.
However, there is clearly something weird going on because those files were explicitly closed. How can I be sure that I closed a file and I can work on it if was very large and I used memmap? That option is there to be used, and would be very convenient :).
Thanks for your help!

@saimn

I think this issue might be the same as #7440 , you are seeing it on Windows, I was seeing it on Mac OS. In #7440 there are simple test cases to reproduce.

I don't fully understand it, but basically the explanation what is goig on was given by @saimn
in #7440 (comment) : hdu_list.close(), which is what is called when you exit the with fits.open(filename) as hdu_list: block, doesn't always close the memmap file (which is kind of like a second open file, see explanation here).

It's not clear to me if this behaviour makes sense, as seen by this issue and #7440 it's very confusing and inconvenient for users, and if Astropy could be changed to a more predictable behaviour, i.e. always closing the memmap in hdu_list.close(), that would be nice. Does anyone have an example where that would cause problems? Why isn't it implemented like that?
Calling gc.collect() in hdu_list.close() is probably not a good idea, no?

Fundamentally I guess the question is what should happen in an ideal world after:

hdulist = fits.open('myfile.fits', memmap=True)
data = hdulist[0].data
hdulist.close()

There are two possibilities here:

D1. data continues to be a memory mapped array and therefore there will be one open file remaining after hdulist.close
D2. data is then switched to be a full copy of the data
D3. data stops working if accessed

It's not clear to me that D2. is desirable since closing a file would then lead to a (potentially) giant memory increase, and it's also not clear how one would swap a memory mapped array for a non-memory-mapped one in-place.

Option D3. would probably break a lot of existing code.

So then I think D1. is the only option in the above snippet.

So then here are (in my view) the possible ways forward assuming D1 works above:

O1. Keep things as-is and just document the issue about closed files
O2. Change memmap to False by default. This is a reasonably big change and could lead to big memory increases for people already using io.fits which is not great if 3.1 was supposed to be the performance release ;)
O3. Change to memmap=False for FITS files that are below a certain size (e.g. 1 or 10MB) since it's not clear that memmap=True actually provides any benefits there, and this addresses the use case of opening many small FITS files.
O4. Keep track of how many memory-mapped open files there are and once the number goes above e.g. 20, we emit a warning that says WARNING: the number of open FITS files with memory mapping is now >20. If you don't need to use memory-mapping, you can explicitly set memmap=False when opening the FITS files or something along those lines.

Personally I would prefer O4 and maybe also O3 (at the same time), plus some documentation, but I'd be curious what other people think?

@astrofrog

@astrofrog - I think I want D3 and would use none of the other options you mention. In Gammapy some users were running out of open files and the process ended, so for them it doesn't help to warn about too many open files, and they can't go in and change lots of Gammapy code to memmap=False, we have to do that for them and make sure files are closed at the end of all of our Map.read and Spectrum.read etc methods.

Like I mentioned in #7440 (comment) I don't understand yet why reliably closing the memmap file shouldn't be possible in that case. Probably there's a line or two that could be added to Table.read or HDUList.__exit__ to close the memmap file in this case where it should be safe to do so.

Is it really not possible to close the memmap file in HDUList.__exit__? Can you give an example snipped of code that people might have now that would break?

If that's the case, then it might still be possible to offer a small API addition to let people do option D3 without having to call gc.collect() like @saimn suggested in #7440 (comment) ?

Otherwise, as some people have already said or started working on, yes, improving the docs to explain a bit about memmap would be very helpful, e.g. I'm still unsure if memmap=True or memmap=False is better when reading FITS BINTABLE into Table objects.

Sorry for confusing things - I think there is indeed a bug in #7440 and maybe here too. I do think:

s = 0
for iter in range(10):
    hdulist = fits.open('myfile.fits', memmap=True)
    data = hdulist[0].data
    hdulist.close()
    s += data.sum()

should work, which means D1 (not D3) is the right behavior (and changing this will break a lot), but at the end of each iteration the memory-mapped object should indeed be closed since data then no longer has any references, and if it doesn't then that's a bug.

So just to be clear, I don't think HDUList.close() should completely close the memory mapping, but the memory map should be garbage collected when it goes out of scope.

@astrofrog

Coming to this a bit late, sorry. D1 is the only possible (and right) behavior imo, but I don't think that there is an issue here with io.fits. And I may be confused but I think that #7440 is a different thing as it concerns Table, not directly io.fits.
If I take @astrofrog's example just above and print the number of opened files with psutil, it doesn't increase.

To show more in detail the behavior, which looks correct to me:

>>> from astropy.io import fits
>>> import psutil
>>> proc = psutil.Process()
>>> print(proc.open_files())
[]
>>> hdulist = fits.open('astropy/io/fits/tests/data/arange.fits', memmap=True)
>>> print(proc.open_files())
[popenfile(path='/home/simon/dev/astropy/astropy/io/fits/tests/data/arange.fits', fd=3, position=8640, mode='r', flags=557056)]

Ok, the file is opened, now let's add a reference to .data:

>>> data = hdulist[0].data
>>> print(proc.open_files())
[popenfile(path='/home/simon/dev/astropy/astropy/io/fits/tests/data/arange.fits', fd=3, position=8640, mode='r', flags=557056), popenfile(path='/home/simon/dev/astropy/astropy/io/fits/tests/data/arange.fits', fd=4, position=8640, mode='r', flags=557056)]

We now have two references, due to the memmap object.

>>> hdulist.close()
>>> print(proc.open_files())
[popenfile(path='/home/simon/dev/astropy/astropy/io/fits/tests/data/arange.fits', fd=4, position=8640, mode='r', flags=557056)]

When closing, HDUList.close checks the number of references to know if it can close the memmap or not. In this case, there is a reference with data, so the file is closed but the memmap is not.

>>> data=None
>>> print(proc.open_files())
[]

If we remove the reference to the array using the memmap object, then everything is closed.

So I think it works as expected. But it is possible that there is a Windows specific issue here ?

@matteobachetti

@matteobachetti - Yes this is certainly a valid point. In the case you reported above, the problem is that Windows does not allow to delete an open file, so it is platform-specific (you can do this on linux, probably on mac os as well) but I agree that it is unexpected. The current behavior could indeed be better documented, the existing FAQ item should be more general than just the "too many open files" issue.

One remark about the scipy case: it is a RuntimeWarning, not a DeprecationWarning.

Yes we can think about what would be the best default behavior, and if we can find a not too painful way to make the transition, but this is really not easy. And we must be careful about not going though a situation that would be worse than the existing one. Leaking a few file descriptors is not a big deal in most cases, things can be worse: at first glance it seems that accessing a closed memmap leads to a segfault (also reproducible with np.memmap).

In [2]: with fits.open('astropy/io/fits/tests/data/arange.fits') as hdul: 
   ...:     data = hdul[0].data 

In [3]: data.base                                                                              
Out[3]: <mmap.mmap at 0x7f1372bbd4b0>

In [4]: data.base.close()                                                                      

In [5]: data                                                                                   
Out[5]: [1]    16269 segmentation fault (core dumped)  ipython

Just FYI:

Yes, there is a file descriptor that remains open for the memmap, as long as there are some references to the memmap.

While it's practically correct this is relying on a CPython implementation detail that objects (that don't participate in cyclic references) are destroyed immediately after the last reference is dropped. However that's not how other Python implementations implement it - it's not even a guarantee in CPython. One should always assume that it can happen anytime between (and including) "last reference to object dropped" and "process ends".

I understand the arguments for backwards compatibility. If you insist on retaining the current behavior, the only correct solution is to deprecate and remove the context handler for io.fits entirely. I have already demonstrated that its current behavior is not only completely meaningless, but extremely misleading to developers and problematic in many cases. Clearly I am not the only developer who is surprised by this, nor the only one who has encountered problems due to the implementation. Just because a choice was made in the past does not mean that it was necessarily correct, or that it reflects current best practices. This is no reason to not fix the issue.

@saimn

@saimn I just looked at the netcdf case you posted. I think that a warning like this would be extremely helpful here in the short term and should probably be added for the next release. I still would argue that this is only a workaround, though, and the underlying issue still needs to be addressed.

Just to attempt to summarize my earlier points, here are my main objections with the current behavior:

The use of a context manager implies to users that all resources are properly being handled, when no such thing is actually occurring in the case of memmap=True. I feel very strongly about this being wrong because it violates both the principle of least surprise.
Given that the memory mapped resources are not handled at all by either the context manager or HDUList.close, there is not an obvious way for users to actually release those resources when they are no longer required. If io.fits requires users to explicitly manage memory mapped resources, then it should provide a way to do this. It should not obfuscate this fact by providing a context manager which actually does nothing.

I'd like to add that the problem also occurs when callers use open and close to explicitly open and release a FITS file, in addition to using with statement.

Example snippets:

    cbv = fits.open('tess_sample_cbv.fits')
    cbv_time = cbv[1].data['Time']
    cbv.close()
    
    os.remove('tess_sample_cbv.fits')  # PermissionError

Yes, that's exactly the current behavior. cbv.close() doesn't actually close the file, and there's no public interface to do so.

cbv.close() doesn't actually close the file

It actually does--the issue is that in the above example there are two handles open to the file: One that was initially used to parse it, and one that is being used to access the data array.

One thing I've thought of doing is adding a warning about this when .close() is called but there is still a handle held on the data, though this only affects users on Windows trying to do something like os.remove() on a file while they're still using the data in that file.

In the above example you can also call .copy() on the array and it should work.

@drdavella

I do agree with @drdavella's points about this being both surprising and problematic for sane resource management.
I'm just not sure what the best solution is. When calling HDUList.close() should we search for any existing references to the data, maybe even force the garbage collector to run (not a bad idea) to ensure any reference cycles are broken and any remaining data references are deleted?

I would actually like this, and to display a warning in this case, because I think continuing to use a the data after closing the file is indicative of slopping programming, albeit understandable since most people don't realize that when they access hdu.data it's not reading the full array into memory.

A warning would help to more easily debug problems like this, and tidy up code. It can also offer an explanation and possible solutions, and/or link to the FAQ (which someone mentioned elsethread should be updated, which I agree).

I wonder how problems does not surface on Linux when the FITS file is removed: I don't mean the PermissionError, but one accessing the underlying (memory-mapped) data.

I did a small test on google colab, running Ubuntu, astropy 4.1, numpy 1.18.5. Somehow one can access mem-mapped data even after the underlying FITS file is removed.

The FITS file is about 50Mb, in case the size matters.

#%%
# !curl -C - -L -o sample_tpf_bak.fits https://mast.stsci.edu/api/v0.1/Download/file/?uri=mast:TESS/product/tess2018206045859-s0001-0000000033877176-0120-s_tp.fits
!ls -l sample_tpf_bak.fits

#%%
from astropy.io import fits
import os

def type_of_original_base(ary):
  if getattr(ary, 'base', None) is None:
    return type(ary)
  else:
    return type_of_original_base(ary.base)


# create a copy to maniuplate and possible remove it
!cp sample_tpf_bak.fits sample_tpf.fits
f_path = 'sample_tpf.fits'

f_tpf = fits.open(f_path, memmap=True)
f_flux = f_tpf[1].data['FLUX']
print('before remove: ', f_flux.shape, type_of_original_base(f_flux))
f_tpf.close()
os.remove(f_path)

!ls -l sample_tpf.fits
# memory-mapped f_flux is still available after f_tpf is closed
print('after remove:  ', f_flux.shape, type_of_original_base(f_flux))
print(f_flux[0])

From the documentation for os.remove():

On Windows, attempting to remove a file that is in use causes an exception to be raised; on Unix, the directory entry is removed but the storage allocated to the file is not made available until the original file is no longer in use.

That is, on Unix systems, the file is "deleted" in that it doesn't show up to the user anymore, but the data is still protected.

A warning would help to more easily debug problems like this, and tidy up code. It can also offer an explanation and possible solutions, and/or link to the FAQ (which someone mentioned elsethread should be updated, which I agree).

Documentation / FAQ would be helpful. A warning could be helpful too but I worry it might end up generating excessive warnings for (I suspect) the predominant cases that do not affect users. The issue arises only when codes try to remove (or move / rename) the underlying FITS file.

On *NIX removing a file simply removes the file from directory entries. This is why os.remove() is a synonym for os.unlink(). The inode for the file is only actually removed when no processes have an open handle to the file.

On Windows the corresponding function DeleteFileA/W has different semantics:

The DeleteFile function fails if an application attempts to delete a file that has other handles open for normal I/O or as a memory-mapped file (FILE_SHARE_DELETE must have been specified when other handles were opened).

The DeleteFile function marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED.

The note about FILE_SHARE_DELETE is interesting; I hadn't heard of that before. Here's some more useful information I found about it. Unfortunately it wouldn't be straightforward to use this in Python without a bit of original C code, or at least ctypes. I think it's possible though--I'll give it a try if I have time and see if it helps at all.

Some fodder for discussion:

Over in the asdf library we have a feature that automatically closes np.memmap arrays when the associated ASDF file is closed. Unfortunately, views over those arrays don't know that the memory map has been closed, and an attempt to use them results in a segfault! Here's what that would look like with astropy.io.fits:

from astropy.io import fits
import numpy as np

fits.HDUList([fits.PrimaryHDU(), fits.ImageHDU(np.zeros(10))]).writeto("test.fits")

with fits.open("test.fits") as hdul:
  view = hdul[-1].data[0:5]
  # Simulate closing the memory map
  hdul[-1].data.base.close()

print("Segfault in 3... 2... 1...", flush=True)

print(view)

So far I haven't come up with a reasonable way to protect against this -- the view object's base array is a memoryview that points to the memory map's address and it has no idea that the address has become invalid. If we can't fix it, I think the segfault is enough of a catastrophe to justify removing the auto-close feature from asdf.

Yep, that's indeed a good reason for not closing the memmap. (#7404 (comment))
So what io.fits does is close the memmap only if there are no other references:

astropy/astropy/io/fits/file.py

Lines 405 to 417 in bb4c197

    
               def _maybe_close_mmap(self, refcount_delta=0): 
        
                   """ 
        
                   When mmap is in use these objects hold a reference to the mmap of the 
        
                   file (so there is only one, shared by all HDUs that reference this 
        
                   file). 
        
                   This will close the mmap if there are no arrays referencing it. 
        
                   """ 
        
                   if (self._mmap is not None and 
        
                           sys.getrefcount(self._mmap) == 2 + refcount_delta): 
        
                       self._mmap.close() 
        
                       self._mmap = None

Navigation Menu

Repository navigation

FITS I/O, WINDOWS only: files remain open despite the use of "with" statement #7404

18 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	def _maybe_close_mmap(self, refcount_delta=0):
	"""
	When mmap is in use these objects hold a reference to the mmap of the
	file (so there is only one, shared by all HDUs that reference this
	file).

	This will close the mmap if there are no arrays referencing it.
	"""

	if (self._mmap is not None and
	sys.getrefcount(self._mmap) == 2 + refcount_delta):
	self._mmap.close()
	self._mmap = None

FITS I/O, WINDOWS only: files remain open despite the use of "with" statement #7404

Description

Activity

pllim commented on Apr 24, 2018

matteobachetti commented on Apr 26, 2018

pllim commented on May 2, 2018

pllim commented on May 2, 2018

Alex-Ian-Hamilton commented on May 2, 2018

matteobachetti commented on May 3, 2018

saimn commented on May 3, 2018

matteobachetti commented on May 9, 2018

cdeil commented on May 10, 2018

astrofrog commented on May 10, 2018

cdeil commented on May 10, 2018

astrofrog commented on May 10, 2018

saimn commented on May 23, 2018

18 remaining items

saimn commented on Apr 15, 2019

MSeifert04 commented on Apr 16, 2019

drdavella commented on Apr 16, 2019

drdavella commented on Apr 17, 2019

orionlee commented on Oct 31, 2020

jdavies-st commented on Nov 2, 2020

embray commented on Nov 2, 2020

embray commented on Nov 2, 2020

orionlee commented on Nov 2, 2020

ayshih commented on Nov 2, 2020

orionlee commented on Nov 2, 2020

embray commented on Nov 3, 2020

eslavich commented on May 7, 2021

saimn commented on May 7, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions