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

feat: return outcome of set and recrypt. #584

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

antondalgren
Copy link

This commits adds a boolean return value from the underlying MMKV library to the JS environment for the set and recrypt calls.

closes #583

This commits adds a boolean return value from the underlying MMKV library to the JS environment for the set and recrypt calls.
@mrousavy
Copy link
Owner

Is there any case where a set could fail? Unless there's some critical memory error or something I feel like this is a bit unnecessary 🤔

@antondalgren
Copy link
Author

The set could fail e.g if there is not enough disk space left.

Follwing is a set of method calls that will show this:
https://github.com/Tencent/MMKV/blob/272e5aa1fb57b4ee8cc0ff37e70b7885d99832a7/Core/MMKV_IO.cpp#L579

auto ret = appendDataWithKey(data, key, isDataHolder);

in appendDataWithKey
https://github.com/Tencent/MMKV/blob/272e5aa1fb57b4ee8cc0ff37e70b7885d99832a7/Core/MMKV_IO.cpp#L797

return doAppendDataWithKey(data, keyData, isDataHolder, static_cast<uint32_t>(keyData.length()));

in doAppendDataWithKey
https://github.com/Tencent/MMKV/blob/272e5aa1fb57b4ee8cc0ff37e70b7885d99832a7/Core/MMKV_IO.cpp#L744-L747

    bool hasEnoughSize = ensureMemorySize(size);
    if (!hasEnoughSize || !isFileValid()) {
        return make_pair(false, KeyValueHolder());
    }

in ensureMemorySize
https://github.com/Tencent/MMKV/blob/272e5aa1fb57b4ee8cc0ff37e70b7885d99832a7/Core/MMKV_IO.cpp#L382-L397

return expandAndWriteBack(newSize, std::move(preparedData))

inexpandAndWriteBack
https://github.com/Tencent/MMKV/blob/272e5aa1fb57b4ee8cc0ff37e70b7885d99832a7/Core/MMKV_IO.cpp#L401C21-L429

        // if we can't extend size, rollback to old state
        if (!m_file->truncate(fileSize)) {
            return false;
        }

        // check if we fail to make more space
        if (!isFileValid()) {
            MMKVWarning("[%s] file not valid", m_mmapID.c_str());
            return false;
        }

As recrypt also will end up in the expandAndWriteBack function, recrypt may also fail.

Without this boolean return value, users of this library may think that the value that was set is guaranteed to be persisted, but in fact we can see that if the file can't be expanded, the write will return false and rollback to the old state without notifying the caller.

@mrousavy
Copy link
Owner

Good point. Hm, I'll think about this and come back soon

Anyways, the PR is good & well implemented, thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return success state for set and recrypt calls
2 participants