Socket concurrency protection

[rawnsley]

If a callback handler for an event such as socket.onOpen calls a method that mutates the callback list, Java will throw a ConcurrentModificationException. The proposed fix makes copies of the lists before invocation, which is a simple change and lock-free.

I'm not sure actually if line 483 is necessary as I think filter already creates a new list. Kotlin appears to differ from C# LINQ in that way, but I don't know if it's guaranteed to be a new list.

Apologies for not including a unit test to expose this but I ran out of time. One of the unit tests in the code base is currently failing for me anyway and also I can't get the project to work reliably in Android Studio. Neither of these points are relevant to the PR, but I thought I would comment in passing.

[codecov-io]

Codecov Report

Merging #67 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master      #67      +/-   ##
============================================
+ Coverage     91.36%   91.38%   +0.01%     
  Complexity      178      178              
============================================
  Files             9        9              
  Lines           533      534       +1     
  Branches         72       72              
============================================
+ Hits            487      488       +1     
  Misses           13       13              
  Partials         33       33

Impacted Files	Coverage Δ	Complexity Δ
src/main/kotlin/org/phoenixframework/Socket.kt	92.72% <100%> (+0.04%)	71 <1> (ø)	⬇️

---

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 2d5c52f...f4f2431. Read the comment docs.

[dsrees]

I used IntelliJ IDEA for development. You could try running the project in that and see if it works for you?

Also, if you could propose a unit test, then maybe i can implement it and see if I can't get it to work. Also which test is failing for you?

[rawnsley]

Oddly it compiles and runs in IntelliJ IDEA just fine and running the ./gradlew test from the command line also works now! Something about importing through Android Studio may have corrupted the gradle setup I think, possibly relating to Java SDK versions.

I've opened a new PR with illustrative unit tests: #68

[AdamGrzybkowski]

would it make sense to simply make all list immutable? I'm talking about all lists in StateChangeCallbacks but also about all list in Socket, Channel etc.

I think it is more error prone if you always have to remember to call .toList()

[dsrees]

@AdamGrzybkowski agreed. I've updated state change callback lists to be read-only. Still need to change other lists.

[AdamGrzybkowski]

@dsrees any update on this? can I help you to speed up things? I see a lot of crashes and I would really like to push a fix for this

[dsrees]

@AdamGrzybkowski wrapping up now. I'll tag you in the PR when it's opened

[dsrees]

Closing in favor of #70 . thanks @rawnsley for the initial request