search

opentelecoms-org / jsmpp

SMPP implemented in Java
Apache License 2.0
230 stars 160 forks source link

Fix deadlocks when trying to a close a session from a session state listener #198

Closed der-ambi closed 8 months ago

der-ambi commented 9 months ago

I was able to fix the testcode in #193 with the patches in this PR.

If you think the ReadWriteLock approach is the right way to go forward with, I would replicate it into the other subclasses of AbstractSessionContext.

PascalSchumacher commented 8 months ago

@der-ambi Shouldn't ReentrantReadWriteLock always be unlocked in a finally block, or am I missing something?

@pmoerenhout What is you opinion on the ReadWriteLock approach?

der-ambi commented 8 months ago

@PascalSchumacher there wasn't much that could have gone wrong between the lock() and unlock() calls, but you are right: best practice is to make sure to unlock() by using a finally clause. I changed the code accordingly.

Once there is an agreement that this is a good approach I will update the other classes accordingly.

pmoerenhout commented 8 months ago

@der-ambi The unlock() in the finally sounds OK to me!

der-ambi commented 8 months ago

@pmoerenhout I have modified all relevant classes to use a ReentrantReadWriteLock instead of synchronized and performed some tests, I couldn't recreate another dead-lock.

pmoerenhout commented 8 months ago

@der-ambi Thanks, I will quickly test it, and merge no problems!

pmoerenhout commented 8 months ago

@der-ambi Thanks for this submission!

PascalSchumacher commented 8 months ago

@pmoerenhout Thank you very much for reviewing and merging this!

A release with this would be great.

pmoerenhout commented 8 months ago

Just released version 3.0.1 of jSMPP.

PascalSchumacher commented 8 months ago

Just released version 3.0.1 of jSMPP.

Thank you very much! 👍