Skip to content
This repository has been archived by the owner on Oct 1, 2018. It is now read-only.

Fix race condition and android bugs resulting in an incomplete/corrupted installation #44

Merged
merged 1 commit into from
Nov 24, 2015

Conversation

andreialecu
Copy link
Contributor

This will make sure the www folder is never left in a corrupted state. If it is corrupted, it will always be recreated on app start. See: #43

Also once I added logging, I noticed a race condition with two installations being started concurrently, and interfering with each other's files.

I noticed that two calls were being made to the install method, and because isInstalling was set from within the thread, both installs would start concurrently. See the small fix in UpdatesInstaller.java.

The two threads would then compete over the www_backup folder, and in my app's case, this would always fail the update because I have a lot of files in the app and file system errors would occur as the two threads were deleting and creating the same files. I believe this was probably the root cause of issue #43, but the strengthening added for that doesn't hurt either.

…ation

This will make sure the www folder is never left in a corrupted state. If it is corrupted, it will always be recreated on app start.

Also fixes a race condition with two installations being started concurrently, and interfering with each other's files. For example both would compete over the www_backup folder, and in my app's case, this would always fail the update because I have a lot of files in the app and file system errors would occur.
@nikDemyankov
Copy link
Member

Thanks! I'm not sure that pluginInternalPrefs.setWwwFolderInstalled(false); is the correct approach for that, feels a little bit hacky. Will check this on monday. Plus, maybe, similar changes would be needed for iOS also. Will do them.

@andreialecu
Copy link
Contributor Author

Well, it is being set to false just before the installation, which would theoretically be correct, because after that point the www folder is not guaranteed to be in a good state - since the installation worker works directly on it.

It is then set back to true immediately after the installation is complete. So it will only be false for a little while, and if something bad happens at that point, the app can recover on the next try and recreate the www folder.

The most important fix here however is that little isInstalling = true line being moved just before the creation of the thread. You can add some logging around there and notice how two installs would be started concurrently prior to this patch.

I think it's a good temporary fix, and unless you want to completely redesign the logic behind installations, it at least made the plugin usable for me and I can now work on deploying my app to the respective stores.

Thanks for your work on it! :)

@nikDemyankov
Copy link
Member

Thank you for all the work :)

I am totally agree with the isInstalling=true changes. No questions in that part. I just want to experiment a bit with the other part.

@nikDemyankov nikDemyankov added this to the v1.1.2 milestone Nov 23, 2015
@nikDemyankov nikDemyankov merged commit c616fe1 into nordnet:master Nov 24, 2015
@nikDemyankov
Copy link
Member

Merged it. But you are right - I need a better way of handling the rollback.

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

Successfully merging this pull request may close these issues.

2 participants