Closed shlusiak closed 2 years ago
cost of potentially missing some newly added keys or traversing into a null value.
@shlusiak Could you give more details about that cost?
Merging #1062 (f046475) into master (5d40917) will increase coverage by
0.01%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #1062 +/- ##
============================================
+ Coverage 65.28% 65.29% +0.01%
Complexity 2218 2218
============================================
Files 122 122
Lines 9957 9960 +3
Branches 1337 1337
============================================
+ Hits 6500 6503 +3
Misses 2945 2945
Partials 512 512
Impacted Files | Coverage Δ | |
---|---|---|
parse/src/main/java/com/parse/ParseObject.java | 60.30% <ø> (ø) |
|
parse/src/main/java/com/parse/ParseTraverser.java | 78.33% <100.00%> (+1.14%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update fd20702...f046475. Read the comment docs.
@mtrezza I think @shlusiak refers that because of the new thread-safe approach, on traversal if there are newly added keys they will be missed in this traversal. On next traversal they will be taken in mind. The funny thing is that on Kotlin I faced this issue a lot using coroutines and patch it by trying not to have multiple traversal in same time.
How does this work technically? Like a read/write queue?
I will reformat the title to use the proper commit message syntax.
Fixed merge conflicts. Previously the app could crash if keys are added or removed while the object is being traversed. With this change, at time of traversal a copy of the keyset is created that is traversed instead, fixing the race conditions of concurrent access. This does not resolve other issues of concurrent access, e.g. adding a key while traversal is happening will miss the newly added key, but also removing a key while traversal is happening would see a key where the value does not exist any more (null
) and tries to traverse into it. That should work, as the traverseInternal checks for that, but it is not ideal.
@mtrezza may you change the title from
fix: create a copy of the ParseObject's keySet when iterating over it.
to
fix: race condition when keys are added or removed while the object is being traversed
Ready for merge?
First-time contributors need a maintainer to approve running workflows.
Can we have the CI running to see the checks passed @mtrezza?
@shlusiak can you lint the code? @L3K0V do you think we should describe how to run lint fix in the contribution guide, or is that clear for developers?
CONTRIBUTING.md is already updated. If you think it should be rephrased, let me know @mtrezza
@L3K0V Does ./gradlew spotlessApply
do more than lint? What do you think about renaming this to ./gradlew lint-fix
, like we have in parse server, dashboard, etc for consistency? Or is spotlessApply
a term an Android developer can be expected to understand?
Ran spotlessApply
, which seems to reorder imports and fixes comments to satisfy lint.
@L3K0V Does ./gradlew spotlessApply do more than lint? What do you think about renaming this to ./gradlew lint-fix, like we have in parse server, dashboard, etc for consistency? Or is spotlessApply a term an Android developer can be expected to understand?
Spotless is widely spread across Java and Kotlin developers. There's standard lint
task which does some other lint checks. I can make it run spotlessApply
. I will consider opening a separate PR.
Meanwhile I will be glad if this and #1058 are merged soon, as they seems to fix a lot of issues.
Looks good! Thanks for the fix!
🎉 This pull request has been released in version 2.0.2
Should fix the
ConcurrentModificationException
in #1061 by creating a local copy before iterating over theParseObject
's keys, with the cost of potentially missing some newly added keys or traversing into anull
value.