[sip-comm-dev] [PATCH] Improve thread safety for SIP telephony related classes


#1

Hi all!

While debugging a problem with SIP calls (see following post), I noticed that
there is no thread safety for the SIP telephony related classes
ProtocolProviderServiceSipImpl, OperationSetBasicTelephonySipImpl and
CallSessionImpl. Since I suspected that this was the cause of my problems, I
tried to improve the situation.

There are (AFAIK) three subsystems using these classes from different threads:

The GUI uses the OperationSetBasicTelephonySipImpl to initiate, answer and
close calls. This in turn invokes methods on the CallSessionImpl and causes
SIP events handled by the ProtocolProviderServiceSipImpl.

The SIP stack sends SIP events to the ProtocolProviderServiceSipImpl. These
are forwarded to the OperationSetBasicTelephonySipImpl, which invokes the
CallSessionImpl.

The JMF sends RTP and controller events to the CallSessionImpl. These events
modify objects which are also used by the other methods invoked by the other
threads.

In the attached patch I've gone for the simple approach and synchronized the
public methods dealing with GUI calls and event handling. This has a certain
risk for introducing deadlocks, but as far as I understand the code, no
synchronized method blocks and waits for some other synchronized method being
called from another thread. I also haven't seen any problems during my test
runs.

While I can't provide a simple reproducible test-case where this patch fixes a
failure, from my tests it seems that it does improve the handling when calls
are initiated and terminated in quick succession.

Regards
Michael Koch

synchronize-sip-methods.patch (11.8 KB)


#2

Hi Michael,

I've committed this one partially (Issue #396). Basically I was afraid
that synchronizing methods that implement JAIN SIP listeners might cause
trouble.

Most of the processXxx() methods result in calls to JAIN SIP so what
bothers me is that locking the JAIN SIP reception thread might make it
impossible to send and lead to a deadlock.

I don't know whether my fears are justified but I'd appreciate it if you
could check the results and let me know whether things still work for
you this way.

Maybe Ranga could also share some advice?

Other than that I've committed the rest of the lines that your patch
contained and ack-ed your effort.

Thanks!
Emil

Koch Michael wrote:

···

Hi all!

While debugging a problem with SIP calls (see following post), I noticed that
there is no thread safety for the SIP telephony related classes
ProtocolProviderServiceSipImpl, OperationSetBasicTelephonySipImpl and
CallSessionImpl. Since I suspected that this was the cause of my problems, I
tried to improve the situation.

There are (AFAIK) three subsystems using these classes from different threads:

The GUI uses the OperationSetBasicTelephonySipImpl to initiate, answer and
close calls. This in turn invokes methods on the CallSessionImpl and causes
SIP events handled by the ProtocolProviderServiceSipImpl.

The SIP stack sends SIP events to the ProtocolProviderServiceSipImpl. These
are forwarded to the OperationSetBasicTelephonySipImpl, which invokes the
CallSessionImpl.

The JMF sends RTP and controller events to the CallSessionImpl. These events
modify objects which are also used by the other methods invoked by the other
threads.

In the attached patch I've gone for the simple approach and synchronized the
public methods dealing with GUI calls and event handling. This has a certain
risk for introducing deadlocks, but as far as I understand the code, no
synchronized method blocks and waits for some other synchronized method being
called from another thread. I also haven't seen any problems during my test
runs.

While I can't provide a simple reproducible test-case where this patch fixes a
failure, from my tests it seems that it does improve the handling when calls
are initiated and terminated in quick succession.

Regards
Michael Koch

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#3

Hi Emil,

Hi Michael,

I've committed this one partially (Issue #396). Basically I was afraid
that synchronizing methods that implement JAIN SIP listeners
might cause
trouble.

Most of the processXxx() methods result in calls to JAIN SIP so what
bothers me is that locking the JAIN SIP reception thread might make it
impossible to send and lead to a deadlock.

I don't know whether my fears are justified but I'd
appreciate it if you
could check the results and let me know whether things still work for
you this way.

Maybe Ranga could also share some advice?

I can understand your concerns. Getting synchronization right is tricky and
hard to debug, and I am never sure if I got it right. I've made the
assumption that the JAIN SIP stack internally never synchronizes on the
object that I synchronize on. Therefore, a deadlock can only occur if I am
calling into the SIP stack from within a synchronized statement
(synchronized on OperationSetBasicTelephonySipImpl), and this call blocks,
waiting for a different thread which in turn wants to synchronize on the
OperationSetBasicTelephonySipImpl (because it wants to notify the object of
an event). This seemed unlikely to me, but of course I was only guessing
(based on the fact that it worked for me when synchronized :slight_smile: It would
really be nice if Ranga could advice.

By the way, I've seen that CallSessionImpl.processSdpAnswer now is
synchronized, while .processSdpOffer is. Was this intentional, or did it
slip through?

Other than that I've committed the rest of the lines that your patch
contained and ack-ed your effort.

Thanks!
Emil

I'm always happy to help.

Regards
Michael Koch

···

Koch Michael wrote:
> Hi all!
>
> While debugging a problem with SIP calls (see following
post), I noticed that
> there is no thread safety for the SIP telephony related classes
> ProtocolProviderServiceSipImpl,
OperationSetBasicTelephonySipImpl and
> CallSessionImpl. Since I suspected that this was the cause
of my problems, I
> tried to improve the situation.
>
> There are (AFAIK) three subsystems using these classes from
different threads:
>
> The GUI uses the OperationSetBasicTelephonySipImpl to
initiate, answer and
> close calls. This in turn invokes methods on the
CallSessionImpl and causes
> SIP events handled by the ProtocolProviderServiceSipImpl.
>
> The SIP stack sends SIP events to the
ProtocolProviderServiceSipImpl. These
> are forwarded to the OperationSetBasicTelephonySipImpl,
which invokes the
> CallSessionImpl.
>
> The JMF sends RTP and controller events to the
CallSessionImpl. These events
> modify objects which are also used by the other methods
invoked by the other
> threads.
>
> In the attached patch I've gone for the simple approach and
synchronized the
> public methods dealing with GUI calls and event handling.
This has a certain
> risk for introducing deadlocks, but as far as I understand
the code, no
> synchronized method blocks and waits for some other
synchronized method being
> called from another thread. I also haven't seen any
problems during my test
> runs.
>
> While I can't provide a simple reproducible test-case where
this patch fixes a
> failure, from my tests it seems that it does improve the
handling when calls
> are initiated and terminated in quick succession.
>
> Regards
> Michael Koch

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail:
dev-help@sip-communicator.dev.java.net


#4

Hi Michael,

Koch Michael wrote:

[snip]

By the way, I've seen that CallSessionImpl.processSdpAnswer now is
synchronized, while .processSdpOffer is. Was this intentional, or did it
slip through?

Right. Well, since you now have commit access, can you do this yourself? :slight_smile:

Emil

···

Other than that I've committed the rest of the lines that your patch
contained and ack-ed your effort.

Thanks!
Emil

I'm always happy to help.

Regards
Michael Koch

Koch Michael wrote:

Hi all!

While debugging a problem with SIP calls (see following

post), I noticed that

there is no thread safety for the SIP telephony related classes
ProtocolProviderServiceSipImpl,

OperationSetBasicTelephonySipImpl and

CallSessionImpl. Since I suspected that this was the cause

of my problems, I

tried to improve the situation.

There are (AFAIK) three subsystems using these classes from

different threads:

The GUI uses the OperationSetBasicTelephonySipImpl to

initiate, answer and

close calls. This in turn invokes methods on the

CallSessionImpl and causes

SIP events handled by the ProtocolProviderServiceSipImpl.

The SIP stack sends SIP events to the

ProtocolProviderServiceSipImpl. These

are forwarded to the OperationSetBasicTelephonySipImpl,

which invokes the

CallSessionImpl.

The JMF sends RTP and controller events to the

CallSessionImpl. These events

modify objects which are also used by the other methods

invoked by the other

threads.

In the attached patch I've gone for the simple approach and

synchronized the

public methods dealing with GUI calls and event handling.

This has a certain

risk for introducing deadlocks, but as far as I understand

the code, no

synchronized method blocks and waits for some other

synchronized method being

called from another thread. I also haven't seen any

problems during my test

runs.

While I can't provide a simple reproducible test-case where

this patch fixes a

failure, from my tests it seems that it does improve the

handling when calls

are initiated and terminated in quick succession.

Regards
Michael Koch

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail:
dev-help@sip-communicator.dev.java.net

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@sip-communicator.dev.java.net
For additional commands, e-mail: dev-help@sip-communicator.dev.java.net


#5

Hi Emil!

> By the way, I've seen that CallSessionImpl.processSdpAnswer now is
> synchronized, while .processSdpOffer is. Was this
intentional, or did it
> slip through?

Right. Well, since you now have commit access, can you do
this yourself? :slight_smile:

Done: trunk@r3226. My first checkin, I hope I didn't break anything :wink: