Fix some reliability isssues for current AsyncStorage implementation #4114
Conversation
ghost
added
the
| @@ -140,6 +140,10 @@ void StorageFileIO::append(const std::string &fileContent) { | |||
| fwrite(fileContent.c_str(), sizeof(char), fileContent.size(), m_storageFile.get()); | |||
| } | |||
There was a problem hiding this comment.
Consider doing the fflush here for safety's sake. Later we should strongly consider reimplementing this entire module. #Resolved
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
#3579
#3392
Issue #1: After storage file is updated, the content of the change does not gets flushed to disk until fclose is called. flocse only happens when asyncStorage module is released. This can leads to data loss or corruption, for example app is shutdown abnormally. To fix it, we call fflush every time after updates are made.
Issue #2: There is a 30 seconds wait for each key value pair I/O operation on the the async load of the storage file content. One of the error path of load throws exception but does not set the load complete event which can cause I/O operation to stuck until 30 seconds wait timed out. We should also set the load complete event before throwing the exception.
Issue #3 The asyncStorage module has a behavior where multiple updates to same key are all written to the disk, so multiple entries for same key can exist on the disk. Those multiple entries are only cleaned up in the next table load when app is initializing, Same thing, any removed keys are added to the file just to be cleaned when next load is happening. This won't work well for scenarios when there are many updates to the same key in a session and the key value is fairly big. Per suggestions from Xbox team, I am making changes to always clear and save the whole table from memory to file when any updates happen.
Next step, I will move the AsyncStorage code as is to community module as it is going to be moved out of RN core soon.
Microsoft Reviewers: Open in CodeFlow