Skip to content

feat: Make ParseObject availableKeys Set a thread safe synchronizedSet #795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

canuludag
Copy link
Contributor

ParseObject gives a ConcurrentModificationException when concurrently modifying the availableKeys HashSet. By making availableKeys synchronized set we can avoid these kind of exceptions.
Looks like there are some open issues that can be related to this -> #154

@codecov
Copy link

codecov bot commented Mar 30, 2018

Codecov Report

Merging #795 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #795   +/-   ##
=========================================
  Coverage     53.35%   53.35%           
  Complexity     1749     1749           
=========================================
  Files           132      132           
  Lines         10268    10268           
  Branches       1426     1426           
=========================================
  Hits           5478     5478           
  Misses         4337     4337           
  Partials        453      453
Impacted Files Coverage Δ Complexity Δ
Parse/src/main/java/com/parse/ParseObject.java 50.95% <100%> (ø) 205 <0> (ø) ⬇️

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 332f02c...ed8a941. Read the comment docs.

@Jawnnypoo Jawnnypoo merged commit a848b04 into parse-community:master Apr 13, 2018
@vovkab
Copy link

vovkab commented Sep 7, 2018

Parse SDK: 1.17.3

I think as a result of this change we are now seeing a new crash:

Fatal Exception: java.lang.StackOverflowError
    at java.util.HashMap.secondaryHash(HashMap.java:350)
    at java.util.HashMap.put(HashMap.java:404)
    at java.util.HashSet.add(HashSet.java:95)
    at java.util.Collections$SynchronizedCollection.add(Collections.java:380)
    at java.util.Collections$SynchronizedCollection.add(Collections.java:380)
    ...
    at java.util.Collections$SynchronizedCollection.add(Collections.java:380)
    at java.util.Collections$SynchronizedCollection.add(Collections.java:380)
    at java.util.Collections$SynchronizedCollection.add(Collections.java:380)
    at com.parse.ParseObject$State$Init.<init>(ParseObject.java:133)
    at com.parse.ParseObject$State$Builder.<init>(ParseObject.java:246)
    at com.parse.ParseObject$State.newBuilder(ParseObject.java:301)
    at com.parse.ParseObject.mergeFromServer(ParseObject.java:982)
    at com.parse.ParseObject.fromJSON(ParseObject.java:707)
    at com.parse.ParseObject.fromJSON(ParseObject.java:684)
    at com.parse.ParseLiveQueryClientImpl.handleObjectEvent(ParseLiveQueryClientImpl.java:322)
    at com.parse.ParseLiveQueryClientImpl.parseMessage(ParseLiveQueryClientImpl.java:253)
    at com.parse.ParseLiveQueryClientImpl.access$000(ParseLiveQueryClientImpl.java:24)
    at com.parse.ParseLiveQueryClientImpl$1.call(ParseLiveQueryClientImpl.java:201)
    at com.parse.ParseLiveQueryClientImpl$1.call(ParseLiveQueryClientImpl.java:199)
    at bolts.Task$4.run(Task.java:357)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
    at java.lang.Thread.run(Thread.java:841)

Possible issues:

  1. We probably should not be synchronizing everywhere, because as a result, we would get synchronizedSet(synchronizedSet(synchronizedSet(...))
  2. If we use synchronizedSet() we should have copied original set instead of wrapping it as is.
  3. We do not synchronize set created in a second constructor:
com.parse.ParseObject.State#State(android.os.Parcel, java.lang.String, com.parse.ParseParcelDecoder)

/* package */ State(Parcel parcel, String clazz, ParseParcelDecoder decoder) {
    ...
    availableKeys = new HashSet<>(available);
}

Are we relying on a side effect that someone accidentally would synchronize it for us?

@RodrigoSMarques
Copy link
Contributor

Hello. I have the same however only on devices with Android 4.x. In no other version do I have problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants