Work around memory errors when using memory mapping on large files in io.fits

[astrofrog]

This is a workaround for #1380, which @Cadair and I ran into while trying to memory map a 30Gb file. The issue is that the available memory address space is less than the size of the file, the OS can in some cases raise a '[Errno 12] Cannot allocate memory' OSError even if it's not actually allocating all the memory (it's just testing if it would work preemptively).

Specifically, mode='readonly' results in the memory-mapping using the ACCESS_COPY mode in mmap so that users can modify arrays (without writing the values back to disk). The solution as described in #1380 is to open the file in mode='denywrite', which at least allows the file to be opened even if the resulting arrays will be truly read-only.

The change here is to modify readarray so that if this specific memory error occurs, it will fall back to reading the arrays as read-only arrays. This also will show a warning so that the user knows what's happening. There is no other way to access the data anyway in this case, so better to have read-only than not at all.

This change should be backward-compatible, as it won't affect anything for cases that worked already. It simply means that some cases that crashed before won't anymore.

Of course, this is io.fits, so who knows ¯\_(ツ)_/¯ - let's see what the CI says.

I'm not sure if this is a bug fix as such, so tentatively milestoning for 3.1, but let me know what you think.

also cc-ing @keflavich and @embray (because of #1380)

[astropy-bot]

Hi there @astrofrog 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

[astrofrog]

The test we added has an issue with a non-closed file:

        if len(not_closed):
            msg = ['File(s) not closed:']
            for name in not_closed:
                msg.append('  {0}'.format(name))
>           raise AssertionError('\n'.join(msg))
E           AssertionError: File(s) not closed:
E             /Users/tom/Dropbox/Code/Astropy/astropy/astropy/io/fits/tests/data/test0.fits

/Users/tom/miniconda3/envs/dev/lib/python3.7/site-packages/pytest_openfiles/plugin.py:108: AssertionError

However we just can't figure out what this is due to so we've added a commit to ignore this error as it is hopefully unimportant.

@drdavella @saimn - if you have any ideas on what could have been causing that issue, please let us know!

[drdavella]

Might be completely off-base here, and I'm not sure whether this will fix the open files issue or not, but it seems like the order of these context handlers might need to be reversed?

[astrofrog]

We've already tried every permutation, and it doesn't help

[drdavella]

After looking at this for a little while myself, I think I'm willing to say that this is probably just due to some weird interaction between mock and mmap, and so skipping the open files test here is okay.

[saimn]

The idea seems fine. In #1380 MAP_NORESERVE was mentioned and looks interesting, but it seems to be not available from Python, so the workaround here is probably the best option.
I'm a bit concerned by the opened file though: it seems that something is keeping an additional reference to the mmap object, but I couldn't find it. In _maybe_close_mmap the refcount is 3, so the mmap is not closed. It could be the mock object, so this would be specific to the test. Running a gc.collect also fixes the issue.
Just to be sure, I guess you don't see a ResourceWarning when reading your file with this PR?

[astrofrog]

@saimn - I don't think we see a resource warning. @Cadair can confirm? I'm pretty sure it's some weird side-effect from the mocking.

[Cadair]

No I didn't see a ResourceWarning when testing it.

[astrofrog]

@saimn - since @Cadair isn't seeing a ResourceWarning, I think it's safe to merge this?

[saimn]

@astrofrog - Yes, looks good

[saimn]

Thanks @astrofrog, I guess we close #1380, as the other option MAP_NORESERVE is not available from Python, and the current option should work in most cases ?

[astrofrog]

@saimn - yes, sounds good!