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.

Sign up for GitHub

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

Fix some reliability isssues for current AsyncStorage implementation #4114

Merged
merged 4 commits into from
Feb 20, 2020
Merged

Fix some reliability isssues for current AsyncStorage implementation #4114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7774041
Fix some reliability isssues for current AsyncStorage implementation
Feb 18, 2020
e4be03a
Change the code to always save whole table to file after update
Feb 19, 2020
031fef4
Change files
Feb 19, 2020
3d22898
format and change
Feb 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions change/react-native-windows-2020-02-19-10-20-40-storagefix.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"type": "prerelease",
"comment": "Fix some reliability isssues for current AsyncStorage implementation",
"packageName": "react-native-windows",
"email": "[email protected]",
"commit": "e4be03a2d921fe401028cda19684e79899ec1b26",
"dependentChangeType": "patch",
"date": "2020-02-19T18:20:40.391Z"
}
44 changes: 26 additions & 18 deletions vnext/ReactWindowsCore/AsyncStorage/KeyValueStorage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ KeyValueStorage::KeyValueStorage(const WCHAR *storageFileName)
m_storageFileLoader = async(launch::async, &KeyValueStorage::load, this);
}

void KeyValueStorage::setStorageLoadedEvent() {
if (!SetEvent(m_storageFileLoaded))
StorageFileIO::throwLastErrorMessage();
}

void KeyValueStorage::load() {
bool saveRequired = false; // this flag is used to indicate whether the file
// needs to be cleaned up.
Expand Down Expand Up @@ -55,33 +60,38 @@ void KeyValueStorage::load() {
break;

default:
throw std::exception("Corrupt storage file. Unexpected prefix on line.");
m_fileIOHelper->clear();
setStorageLoadedEvent();
throw std::exception("Corrupt storage file. Unexpected prefix on line. Storage file cleared.");
break;
}
}
}

if (saveRequired) // cleanup the AOF by dumping the in memory map
{
m_fileIOHelper->clear();
stringstream cleanedUpFile;
saveTable();
}
setStorageLoadedEvent();
}

for (auto const &entry : m_kvMap) // convert in memory map to a string
{
string key = entry.first;
string value = entry.second;
void KeyValueStorage::saveTable() {
m_fileIOHelper->clear();
stringstream cleanedUpFile;

escapeString(key);
escapeString(value);
for (auto const &entry : m_kvMap) // convert in memory map to a string
{
string key = entry.first;
string value = entry.second;

cleanedUpFile << KeyPrefix << key << '\n' << ValuePrefix << value << '\n';
}
escapeString(key);
escapeString(value);

m_fileIOHelper->append(cleanedUpFile.str());
cleanedUpFile << KeyPrefix << key << '\n' << ValuePrefix << value << '\n';
}

if (!SetEvent(m_storageFileLoaded))
StorageFileIO::throwLastErrorMessage();
m_fileIOHelper->append(cleanedUpFile.str());
m_fileIOHelper->flush();
}

void KeyValueStorage::waitForStorageLoadComplete() {
Expand Down Expand Up @@ -134,20 +144,18 @@ void KeyValueStorage::multiSet(const vector<tuple<string, string>> &keyValuePair

if (fUpdateStorageFile) {
// write the new keys to the file
m_fileIOHelper->append(appendEntry.str());
saveTable();
}
}

void KeyValueStorage::multiRemove(const vector<string> &keys) {
waitForStorageLoadComplete();
stringstream ss;

for (auto const &k : keys) {
ss << KeyPrefix << k << '\n' << RemovePrefix << '\n';
m_kvMap.erase(k);
}

m_fileIOHelper->append(ss.str());
saveTable();
}

void KeyValueStorage::multiMerge(const vector<tuple<string, string>> &keyValuePairs) {
Expand Down
4 changes: 3 additions & 1 deletion vnext/ReactWindowsCore/AsyncStorage/KeyValueStorage.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ class KeyValueStorage {
static const uint32_t EstimatedValueSize = 200;
static const char KeyPrefix = '$';
static const char ValuePrefix = '%';
static const char RemovePrefix = 'R';
static const char RemovePrefix = 'R'; // Keep RemovePrefix to be backward compatible for the storage file format

private:
std::map<std::string, std::string> m_kvMap;
Expand All @@ -43,6 +43,8 @@ class KeyValueStorage {
private:
void load();
void waitForStorageLoadComplete();
void setStorageLoadedEvent();
void saveTable();
};
} // namespace react
} // namespace facebook
4 changes: 4 additions & 0 deletions vnext/Shared/AsyncStorage/StorageFileIO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ void StorageFileIO::append(const std::string &fileContent) {
fwrite(fileContent.c_str(), sizeof(char), fileContent.size(), m_storageFile.get());
}
Copy link
Contributor

@kmelmon kmelmon Feb 18, 2020 •
edited by ddalp
Loading

Choose a reason for hiding this comment

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

Consider doing the fflush here for safety's sake. Later we should strongly consider reimplementing this entire module. #Resolved

Copy link
Collaborator

@NickGerleman NickGerleman Feb 19, 2020 •
edited by ddalp
Loading

Choose a reason for hiding this comment

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

Hal is adding a sqlite backed module with his Win32 Playground change that should also work for UWP. It sounded like there was some interest in this module becoming the basis for the community module instead of our current? #Resolved

Copy link
Collaborator

@NickGerleman NickGerleman Feb 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We should try to leave implementation consistent once we have a community module though. Unless we add logic for migration, any change will require apps to drop user data when upgrading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I am going to add a tracking issue for migrating to SqlLite after move to community module with current implementation (with this fix).

In reply to: 381317708 [](ancestors = 381317708)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the data drop issue will exist regardless whether the code is in RNW or community module, it will break once we migrate to sqlLite unless we add logic to read the storage file and save to sqlLite. I think that logic should be pretty simple to implement.

In reply to: 381319360 [](ancestors = 381319360)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a flush function separately in this module, so in case caller can do multiple append() followed by one flush().

In reply to: 380996960 [](ancestors = 380996960)


void StorageFileIO::flush() {
fflush(m_storageFile.get());
}

void StorageFileIO::throwLastErrorMessage() {
char errorMessageBuffer[IOHelperBufferSize + 1] = {0};
#ifdef LIBLET_BUILD // This is silly and makes our lives more difficult while
Expand Down
1 change: 1 addition & 0 deletions vnext/Shared/AsyncStorage/StorageFileIO.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class StorageFileIO {
void append(const std::string &fileContent);
void resetLine();
bool getLine(std::string &line);
void flush();

static void throwLastErrorMessage();

Expand Down