Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set error if mmap address space is exhausted #3437

Merged
merged 5 commits into from Apr 21, 2016

Conversation

mrackwitz
Copy link
Contributor

This is based on the assumption that the address space will be exhausted most commonly either on trying to map a Realm database which doesn't fit in the left address space or on committing a writing transaction when the memory is resized, while this could also happen on (auto-)refreshes.
Fixes #2269.

/c @jpsim @bdash @tgoyne

mrackwitz added a commit that referenced this pull request Apr 14, 2016
@@ -8,7 +8,7 @@ x.x.x Release notes (yyyy-MM-dd)

### Enhancements

* None.
* Fail with `RLMErrorAddressSpaceExhausted` if unable to acquire more mmap memory.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return a recoverable RLMErrorAddressSpaceExhausted error rather than crashing when failing to mmap in Realm initialization or write commit.

@mrackwitz mrackwitz force-pushed the mr-fail-address-space-exhausted branch from d8c12bd to 6d49615 Compare April 14, 2016 17:06
mrackwitz added a commit that referenced this pull request Apr 14, 2016
@jpsim
Copy link
Contributor

jpsim commented Apr 14, 2016

Some context from a previous conversation: although this exception can be thrown in advance_read, it's not easily recoverable there because all accessors will become invalid. Also, advance_read can happen implicitly in so many situations that it would be impractical to allow error handling there.

One solution to this might be to allow users to register custom error handlers that we would invoke for them, but the recovery options would still be very limited, so probably not worthwhile implementing unless there's a strong demand for it.

I'll wait for the compile issues to be sorted out before doing a full review.

@mrackwitz mrackwitz force-pushed the mr-fail-address-space-exhausted branch from 6d49615 to a748e88 Compare April 14, 2016 17:14
mrackwitz added a commit that referenced this pull request Apr 14, 2016
@mrackwitz
Copy link
Contributor Author

This passes now on CI. (Jenkins failure is due to the device build being aborted.)

@tgoyne
Copy link
Member

tgoyne commented Apr 15, 2016

It'd be nice to have a test that verifies that we actually get the correct error and that it's recoverable. An idea for how to do this is:

create realm file with a 1MB string, close realm
size = 1024 * 1024 * 1024
while (size >= 1024 * 1024) {
    kern_return_t ret = vm_allocate(size)
    if (ret == KERN_NO_SPACE)
        size /= 2
    else
        add allocation to list to free
}
try to open realm, assert that expected error is given
deallocate all allocations above
try again to open realm and verify that it works

This would obviously need to be 32-bit only. It may turn out to be unreasonably slow to run it as part of the normal test suite, but it'd still be good to at least do a one-time verification.

@mrackwitz
Copy link
Contributor Author

@tgoyne: Thanks for the very detailed proposal how to test that. I was thinking about that and @ironage was also giving me some good feedback. I'll report back, once I was able to reproduce it, whether it is actually recoverable and if it's a good idea to test that continuously.

@jpsim
Copy link
Contributor

jpsim commented Apr 15, 2016

👍 if testing confirms this works

@mrackwitz
Copy link
Contributor Author

mrackwitz commented Apr 18, 2016

I think, I could reproduce it, assuming that my test case makes sense. 🙈
It didn't seemed like it was a huge performance regression for the test suite at least locally for me, but it might look different for our CI VMs.

@mrackwitz
Copy link
Contributor Author

I think it would be good if someone could review my test case and give a 👍 if everything is alright.

vm_address_t address;
vm_size_t size;
};
typedef struct VirtualMemoryChunk VirtualMemoryChunk;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This typedef shouldn't be necessary since this code is in an Objective-C++ file.

@bdash
Copy link
Contributor

bdash commented Apr 19, 2016

👍

1 similar comment
@jpsim
Copy link
Contributor

jpsim commented Apr 20, 2016

👍

@mrackwitz mrackwitz force-pushed the mr-fail-address-space-exhausted branch from 7f4b10e to a05de65 Compare April 20, 2016 12:45
mrackwitz added a commit that referenced this pull request Apr 20, 2016
@mrackwitz mrackwitz force-pushed the mr-fail-address-space-exhausted branch 2 times, most recently from 45823b7 to 40563fd Compare April 20, 2016 22:38
@mrackwitz mrackwitz force-pushed the mr-fail-address-space-exhausted branch from 40563fd to 124856d Compare April 21, 2016 10:19
@mrackwitz mrackwitz merged commit 9b9f997 into master Apr 21, 2016
@mrackwitz mrackwitz deleted the mr-fail-address-space-exhausted branch April 21, 2016 12:00
realm-ci pushed a commit that referenced this pull request Apr 21, 2016
* master:
  [Test] Try to exhaust adress space on device/32-bit sim only
  [Test] Check that AddressSpaceExhausted is correctly rethrown
  [CHANGELOG] Add entry for #3437
  Catch AddressSpaceExhausted and set error
  [Doc] Fix inconsistency for Error. IncompatibleLockFile
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set NSError on Realm initialization failure due to inability to mmap
4 participants