fix: race condition when keys are added or removed while the object is being traversed

[shlusiak]

Should fix the ConcurrentModificationException in #1061 by creating a local copy before iterating over the ParseObject's keys, with the cost of potentially missing some newly added keys or traversing into a null value.

[mtrezza]

cost of potentially missing some newly added keys or traversing into a null value.

@shlusiak Could you give more details about that cost?

[codecov]

Codecov Report

Merging #1062 (f046475) into master (5d40917) will increase coverage by 0.01%.
The diff coverage is 100.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%)	⬆️

---

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.

[azlekov]

@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.

[mtrezza]

How does this work technically? Like a read/write queue?

[parse-github-assistant]

I will reformat the title to use the proper commit message syntax.

[parse-github-assistant]

Thanks for opening this pull request!

• ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

[shlusiak]

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.

[azlekov]

LGTM

[azlekov]

@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

[mtrezza]

Ready for merge?

[azlekov]

First-time contributors need a maintainer to approve running workflows.

Can we have the CI running to see the checks passed @mtrezza?

[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?

[azlekov]

CONTRIBUTING.md is already updated. If you think it should be rephrased, let me know @mtrezza

[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?

[shlusiak]

Ran spotlessApply, which seems to reorder imports and fixes comments to satisfy lint.

[azlekov]

@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.

[mtrezza]

Looks good! Thanks for the fix!

[parseplatformorg]

🎉 This pull request has been released in version 2.0.2