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

Already on GitHub? Sign in to your account

TCAP dialogs put-get collision in TCAPProviderImpl #107

Closed
Oleg-Afanasiev opened this Issue Jun 2, 2016 · 6 comments

Comments

Projects
None yet
4 participants

Type: Fix
Description: TCAP fragments of code "... dialogs.get(dialogId) ..." in TCAPProviderImpl need to be inside of synchronized blocks. Because collisions of getting by dialogId can happen before this value will be put. This collisions were detected in high loaded tests with several hundreds of threads on multiprocessor machine.
I managed to fix this problem and attaching both fixed file and old one.

TCAPProviderImpl.java.txt
TCAPProviderImpl-fixed.java.txt

Owner

deruelle commented Jun 2, 2016

Thanks @Oleg-Afanasiev for the report and contribution !

Can you please sign the contributor license agreement at http://www.telestax.com/open-source/#Contribute and also do a formal pull request as decribed in our Open Source Playbook ?

Collaborator

vetss commented Jun 9, 2016

Hello @Oleg-Afanasiev,

Let's continue discussing in #110.
We need to have the best solution for it.

@vetss vetss added the enhancement label Jun 9, 2016

@vetss vetss added this to the 7.1.0 milestone Jun 9, 2016

@vetss vetss self-assigned this Jun 9, 2016

have you consider using FastMap.shared API as in concurrentHashMap, trying to leverage putIfAbsent?. The fix involves a system/stack level lock to protect the "getPreviewDialog" where the actual write happens, this may introduce high thread contetnion(consider reducing the scope of the lock for both the sync object and the code block).

Collaborator

vetss commented Dec 21, 2016

Hello @jaimecasero

we used FastMap for much time trying to achieve max productivity (as FastMap declares very high scalabiliy) by usage of locking for writing and reading without locking. But as for now we have confirmations that there are collisions there.
Usage of FastMap.shared() (as for its description) will not fix that collision problem. And "putIfAbsent()" we have not tried, I have no idea how it may help us.
I am thinkg of leverage of ConcurrentHashMap collection and remove our synch() blocks for them.

As for a suggested fix, I do not like such big synch() blocks too.

jaimecasero commented Dec 21, 2016 edited

ConcurrentHashMap collection==FastMap.shared() -> the key of these concurrent collections is not the implementation (i agree using shared alone will not make any difference), but the additional API exposed. If you dont use the "putIfAbsent" method from a ConcHashMap/shared, instead of regualr "put" there wont be much difference (that true even for CHM).
The magic of putIfAbsent is that the key presence check is done atomically inside the collection impl.

Also there are new features in Java8.computeIfAbsent or Guava.MapMaker collections that may be worth visitins to prevent the systemWide lock, as explained in:
http://stackoverflow.com/questions/3752194/should-you-check-if-the-map-containskey-before-using-concurrentmaps-putifabsent

Collaborator

vetss commented Jan 19, 2017

Hello @jaimecasero @Oleg-Afanasiev

thanks for raising an issue and discussing of it. I have committed updates for the described problem. Please check commits list in #110. I have tried to remove of synchronized() blocks and switched instead of it to ConcurrentHashMap that may help us in collisions.
Updates are committed into master branch.

@Oleg-Afanasiev , can you please check the code at your tests for checking if a problem is fixed or not.

@vetss vetss modified the milestones: Sergey-8.0-sprint1, 8.0.0 Jul 23, 2017

@vetss vetss closed this Aug 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment