Completing getId call with the disk reads on the caller thread. #1570
Conversation
ankitaj224
commented
May 18, 2020
ankitaj224
commented
- Fixing delete API to access persisted FID using cross process safe locks.
- Minor code cleanup
Access the data shared by multiple threads only after acquiring cross-process and multi-thread safe locks.
Also, fixing delete API to access persisted FID using cross process safe locks.
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (df669eaa) is created by Prow via merging commits: b1172c9 a5f6558. |
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (df669eaa) is created by Prow via merging commits: b1172c9 a5f6558. |
| @@ -520,7 +509,7 @@ private PersistedInstallationEntry fetchAuthTokenFromServer( | |||
| * storage. | |||
| */ | |||
| private Void deleteFirebaseInstallationId() throws FirebaseInstallationsException, IOException { | |||
| PersistedInstallationEntry entry = persistedInstallation.readPersistedInstallationEntryValue(); | |||
| PersistedInstallationEntry entry = getPrefsWithGeneratedIdMultiProcessSafe(); | |||
There was a problem hiding this comment.
This will create a new fid and delete it. I think it is better the way it was.
There was a problem hiding this comment.
Reason for this: If there is one thread that is triggering getId or getToken and another triggering delete. If the delete read is not multi-process safe, and tries to read the same data locked by other thread acquiring CrossProcessLock - could lead to OverLappingFileLockException
There was a problem hiding this comment.
As discussed offline: created and used cross-process safe read prefs method.
| Task<String> task = addGetIdListener(); | ||
| backgroundExecutor.execute(this::doGetId); | ||
| return task; | ||
| synchronized (lockGetFid) { |
There was a problem hiding this comment.
I see that you want to emulate the old behavior of this being serialized on a sinle thread, but I don't know of adding an additional synchronization block is the right way to go.
For one thing, it isn't multi-process safe.
There was a problem hiding this comment.
Actually, everything doGetId() does it already thread and process safe, so this doesn't need its own protection, does it?
There was a problem hiding this comment.
I missed that local variables are thread safe. PTAL
| @@ -73,6 +73,10 @@ | |||
|
|
|||
| /* used for thread-level synchronization of generating and persisting fids */ | |||
| private static final Object lockGenerateFid = new Object(); | |||
|
|
|||
| /* used for thread-level synchronization of getting a fid. */ | |||
There was a problem hiding this comment.
How about this comment:
/** Thread level synchronization for reading FID from disk. */
...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java
Outdated
Show resolved
Hide resolved
...ase-installations/src/main/java/com/google/firebase/installations/FirebaseInstallations.java
Outdated
Show resolved
Hide resolved
* Cache FID to avoid multiple disk reads. * Addressing Rayo's comments.
