react-native-mmkv icon indicating copy to clipboard operation
react-native-mmkv copied to clipboard

feat: return outcome of set and recrypt.

Open antondalgren opened this issue 11 months ago • 3 comments

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

closes #583

antondalgren avatar Sep 26 '23 08:09 antondalgren

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 🤔

mrousavy avatar Sep 26 '23 10:09 mrousavy

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.

antondalgren avatar Sep 26 '23 11:09 antondalgren

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

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

mrousavy avatar Sep 27 '23 11:09 mrousavy