Skip to content
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

Fix TCAP dialog put-get collision in TCAPProviderImpl #109

Conversation

Oleg-Afanasiev
Copy link

Hello,

This PR is solution for issue#107. TCAP->TCAPProviderImpl->onMessage - was added synchronized blocks.

Brgds,
Oleg Afanasiev

@deruelle deruelle added this to the 7.1.0 milestone Jun 2, 2016
@deruelle
Copy link
Member

deruelle commented Jun 2, 2016

Thanks @Oleg-Afanasiev for signing the CLA and for the PR ! @vetss will review the PR

@deruelle
Copy link
Member

deruelle commented Jun 2, 2016

Just a quick comment, instead of using synchronized on dialogs, can't we use a ConcurrentHashMap instead this would perform better than using synchronized keyword especially with places like https://github.com/Oleg-Afanasiev/jss7/blob/2e4a025ebab65377970be41347467f04055e15bd/tcap/tcap-impl/src/main/java/org/mobicents/protocols/ss7/tcap/TCAPProviderImpl.java#L584 where we only do get...

@vetss
Copy link
Contributor

vetss commented Jun 3, 2016

Hello @Oleg-Afanasiev,

can you please comment your update ? I mean why do you think that we need to inlude "di = this.dialogs.get(dialogId);" into synch() blocks. Do you have some concrete experiense that the current code brings you any problems that you have detected ?

I have also created an issue for discussing of synching issues
#110
Feel free to add there any comments.

@Oleg-Afanasiev
Copy link
Author

Hello. Thanks for discussing. Yes, we have concrete problems with "this.dialogs.get(dialogId)". We made high load tests under competitive environment. But we couldn't tested without fixing TCAP library (placing this fragments of code into synch() blocks), because put-get collisions were happen as described in #107.

@vetss
Copy link
Contributor

vetss commented Jun 9, 2016

Hello @Oleg-Afanasiev,
ok, thanks for a valuable info.

As for your concete commit I need just to note that it is better to remove code like:
dialogId = Utils.decodeTransactionId(tcm.getDestinationTransactionId());
from "synchronized (this.dialogs)" block and leave their only code like "this.dialogs.get(dialogId)".

Let's continue discussing in #110.
I am thinking that may be it will be more effective to use instead of FastMap<Long, DialogImpl> another collection like ConcurrentHashMap that can be used without synchronized blocks.

@deruelle deruelle modified the milestone: 8.0.0 Feb 20, 2017
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants