Fix a ConcurrentModificationException in ObjectValue overlays #3062
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
Hi @bernard-kms. Thanks for your PR. I'm waiting for a firebase member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@googlebot I signed it! |
|
Hi @bernard-kms Thank you for the PR! This does look like ObjectValue has a concurrent issue. What I am worried about is that this should not happen, because on the write path users are not supposed to get hold of a Do you mind sharing how you are making the writes? |
|
Hi! Thanks for the quick response. Actually I also confused with this race happened. Crashlystics logs have the following in common:
Making writes are somewhat simple: val documentId = model.getId()
val reference = getCollection(userUid).document(documentId)
reference.set(model.getHashMap()), SetOptions.merge())And making reads are: val reference = getCollection(userUid).document(documentId)
return withContext(Dispatchers.IO) {
val snapshot = reference.get().await()
if (snapshot.exists()) {
Model.fromSnapshot(snapshot)
...// Model.kt
data class Model(
...
fun fromSnapshot(doc: DocumentSnapshot): Model {
val data = doc.data
if (data != null) {
return Model(...All async tasks are handled with kotlin coroutines. |
|
I am not very sure, but I think the race condition is within Do you mind trying to lock on |
|
Sure! I will try to repro this issue with a test code and see what happens. |
|
I did some more investigations, it seems more likely caused by I tried to reproduce the situation with unit test and the following is the closest repro to got similar stack trace from Crashlystics: ObjectValue objectValue = new ObjectValue();
int i = 0;
while (i++ < 1000) {
objectValue.set(field(i + ""), fooValue);
new Thread(new Runnable() {
@Override
public void run() {
for (Map.Entry<String, Value> entry : objectValue.getFieldsMap().entrySet()) {}
}
}).start();
}I also tried lock variable Please let me know if I missed something. Edit: In my thought, synchronized method would be minimal changes to resolve the issue. |
|
Thanks for the analysis and research! My main concern is the way you tested it (making get and set concurrently) should not happen. With a
|
|
Oh now I completely realize what it means and your concern. I will make |
|
It seems not-too-small ObjectValue overlay actually leads to ObjectValue objectValue = new ObjectValue();
int i = 0;
objectValue.set(field("a.b.c"), fooValue);
objectValue.set(field("a.b.d"), fooValue);
objectValue.set(field("f.g"), fooValue);
objectValue.set(field("h"), fooValue);
while (i++ < 1000) {
new Thread(new Runnable() {
@Override
public void run() {
for (Map.Entry<String, Value> entry : objectValue.getFieldsMap().entrySet()) {}
}
}).start();
}Current PR fix this repro, locking |
|
/retest |
Coverage ReportAffected SDKs
Test Logs
NotesHTML coverage reports can be produced locally with Head commit (dd84bb47) is created by Prow via merging commits: 69fb475 ce792be. |
Binary Size ReportAffected SDKs
Test Logs
NotesHead commit (dd84bb47) is created by Prow via merging commits: 69fb475 ce792be. |
|
@bernard-kms: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
I observed ConcurrentModificationException in some race conditions. Following stack trace logged via Crashlystics;
It seems that rapid setting/getting overlays in ObjectValue could result ConcurrentModificationException.