pilgr / Paper

Paper is a fast NoSQL-like storage for Java/Kotlin objects on Android with automatic schema migration support.
Apache License 2.0
2.34k stars 234 forks source link

multi threads DbStoragePlainFile.createPaperDir() will throw new RuntimeException("Couldn't create Paper dir: " + mDbPath); #114

Closed bigkun closed 4 years ago

bigkun commented 6 years ago

multi threads DbStoragePlainFile.createPaperDir() will throw new RuntimeException("Couldn't create Paper dir: " + mDbPath); because “boolean isReady = new File(mDbPath).mkdirs();” will return false.

fix:

 private void createPaperDir() {
        synchronized (DbStoragePlainFile.class) {
            if (!new File(mDbPath).exists()) {
                boolean isReady = new File(mDbPath).mkdirs();
                if (!isReady) {
                    throw new RuntimeException("Couldn't create Paper dir: " + mDbPath);
                }
            }
        }
    }
pilgr commented 6 years ago

Thanks, now I see where the issue may happen. Did you see the issue it in your app? One moment is that createPaperDir should be synchronized by current DbStoragePlainFile instance, not by DbStoragePlainFile class.

jeff-huston commented 5 years ago

I believe a race condition in our app ran into this.

We had a first operation that would complete, and then it would kick off a "save" on another thread (this "save" would destroy() and then insert() into Paper).

A second operation would run immediately after the first operation and try to read by calling getAllKeys() in Paper.

getAllKeys() appears to surprisingly make a call to createPaperDir() within assertInit(). Within this createPaperDir() we were seeing this RuntimeException, which we suspect was occurring when the aforementioned functions would happen to get called in this order:

  1. destroy() from the first operation
  2. if (!new File(mDbPath).exists()) from within createPaperDir() in the second operation
  3. insert() from the first operation
  4. new File(mDbPath).mkdirs() from within createPaperDir() in the second operation

Clearly the race condition we created in our app could have caused different, even harder-to-diagnose problems, even if this reported Issue didn't exist. Just wanted to share our experience.

svenoaks commented 5 years ago

I see this crash happening in the beta version of my app with only a couple thousand users. I think "Reading/writing for different keys can be done in parallel" is not really safe at the moment.

wassil commented 4 years ago

Happens to our SDK as well. The SDK is used to report analytic events, so it's definitely possible for 2 threads to use the database at the same time. I'm also able to reproduce this easily in unit tests, just spawn 10 threads in for loop and use database in another for loop 10 times.

wassil commented 4 years ago

@pilgr seems like bigkun suggested a correct fix 2 years ago, is this getting fixed? Should I do it?

pilgr commented 4 years ago

@wassil thanks but I'm already working on the fix, please review PR once it's ready

pilgr commented 4 years ago

Please review the PR with the fix https://github.com/pilgr/Paper/pull/168

pilgr commented 4 years ago

Fixed and merged to master, included in the new version 2.7.1