Fix Concurrent Modification Exception in DefaultChannel on setNetwork…

shadyabarada commented 1 year ago

…RequestsAllowed

Please have a look at our guidelines for contributions and consider the following before you submit the PR:

[x] Has CHANGELOG.md been updated?
[x] Are tests passing locally?
[x] Are the files formatted correctly?
[x] Did you add unit tests?
[x] Did you test your change with either the sample apps that are included in the repository or with a blank app that uses your change?
[x] Did you check UI tests on the sample app? They are not executed on CI.

Description

This issue is occuring because we are iterating on a hashMap that at some point can be modified by other running threads. This will cause a concurrent modification exception which is very common in data structures with iterators. One solution is to use the ConcurrentHashMap instead of HashMap given that our map is exposed to multiple modifications in our code.

Related PRs or issues

AB#96392 https://github.com/microsoft/appcenter-sdk-android/issues/1666/

Misc

Add what's missing, notes on what you tested, additional thoughts or questions.

shadyabarada commented 1 year ago

@ternovtsi

In fact the iterator with some data structures like a hash map are not protected from concurrent modification exceptions. You can check my ticket where I added a sample code that does reproduce it.

Regarding this ticket, you can now test again, the issue apparently was from the test key names - now whenever you use hashmap data structure type with the msGroupStates object, your test will fail.

Let me know if you face further issues.

I also believe there is no official formatting for Java, there is just preferences, maybe we should create a shared formatting xml file for the whole team to use so that we don't find formatting changes during PR and we all be aligned on one.

ghost commented 1 year ago

@ternovtsi

In fact the iterator with some data structures like a hash map are not protected from concurrent modification exceptions. You can check my ticket where I added a sample code that does reproduce it.

Regarding this ticket, you can now test again, the issue apparently was from the test key names - now whenever you use hashmap data structure type with the msGroupStates object, your test will fail.

Let me know if you face further issues.

I also believe there is no official formatting for Java, there is just preferences, maybe we should create a shared formatting xml file for the whole team to use so that we don't find formatting changes during PR and we all be aligned on one.

You're right it is protected only when removing items, sorry forgot that

ghost commented 1 year ago

Also, remove unused imports, please)

shadyabarada commented 1 year ago

@ternovtsi removed extra spaces and imports, and regarding the formatting I don't see there difference between my code and the shared doc. could you please point it out in case I missed it.

ghost commented 1 year ago

@ternovtsi removed extra spaces and imports, and regarding the formatting I don't see there difference between my code and the shared doc. could you please point it out in case I missed it.

Everything i mentioned is in doc. For example: 7.9 try catch, 7.4 if statement

microsoft / appcenter-sdk-android