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 for Can't activate two sip RA entities. #28

Merged
merged 1 commit into from Dec 16, 2016

Conversation

Projects
None yet
4 participants
@yarel79
Contributor

yarel79 commented Oct 30, 2016

This is a proposed fix for issue/27.

Class loading influence might be a separate thing, and it can cover the root cause of SIP RA issue. If SIP RA entities are created from different class loaders, then the issue may not appear.

SipFactory is a singleton and it keeps track of existing SipStack instances created by SipFactory, basically the same stack instance is returned in case:
a) sip stack's name (provided in Properties) matches the name of one of already created stacks
b) sip stack's IP (provided in Properties) matches IP of already cretead stacks

SipFactory first relies on javax.sip.IP_ADDRESS, then on javax.sip.STACK_NAME.

Since SipResourceAdaptor provided javax.sip.IP_ADDRESS, then the same stack was returned by SipFactory, but later on in raActive(), new provider and listener were created and added to existing stack (which already had provider/listener set).

Now, different class loaders implies different singletons (SipFactory) created. The other singleton knows nothing about first stack crated, thus it will create a new stack.

The fix just removes IP_ADDRESS from properties passed to SipFactory. Still it allows to create a listening point passed to RA configuration.

There is a unit test provided to verify the fix.

@deruelle

This comment has been minimized.

Member

deruelle commented Oct 30, 2016

Thanks @yarel79. @jaimecasero @SergeyLee can you review ?

@jaimecasero

Focused and good solution. Test is good but introduces lots of new mocks libs,which im not sure are worthy

@@ -1619,7 +1635,7 @@ public void unsetResourceAdaptorContext() {
this.sleeEndpoint = null;
this.eventLookupFacility = null;
this.tracer = null;
this.tracer = null;
this.providerWrapper = null;

This comment has been minimized.

@jaimecasero

jaimecasero Oct 31, 2016

seems fair compared to counterpart setResourceAdaptorContext.. Hope it will not hurt later

* Let's make sure no javax.sip_IP_ADDRESS is not used for SipStack creation, later on Listening Point will
* be created as per provided IP_ADDRESS .
*/
if (properties.getProperty(SIP_BIND_ADDRESS) != null) {

This comment has been minimized.

@jaimecasero

jaimecasero Oct 31, 2016

Agree, this should be enforced. Comment is clear enough to understand the intention of the code. Dow e need to add maybe a logger trace so the integrator/admin gets a warning about the address being ignored at "stack" level?

@@ -25,6 +25,24 @@
<groupId>org.mobicents.servers.jainslee.core</groupId>
<artifactId>fault-tolerant-ra-api</artifactId>
</dependency>
<!-- For testing purposes -->
<dependency>
<groupId>org.mockito</groupId>

This comment has been minimized.

@jaimecasero

jaimecasero Oct 31, 2016

we have both Restcomm level poms (https://github.com/RestComm/parent/blob/2.x/pom.xml), and per project level poms https://github.com/RestComm/jain-slee.sip/blob/master/pom.xml). We may evaluate to move these dependencies to a higher scope...

This comment has been minimized.

@SergeyLee

SergeyLee Dec 16, 2016

Contributor

I don't see any mockito/powermock in RestComm parent pom. So I relocate dependencies in SIP RA parent pom.

private static final int SIP_RA2_PORT = 5059;
private static final String STACK_ADDRESS = "127.0.0.1";
// common mocked tracer
private static Tracer tracer1 = mock(Tracer.class);

This comment has been minimized.

@jaimecasero

jaimecasero Oct 31, 2016

do we really need to mock Tracer, and "info" method. I cant see any assertion on that... I can see is neccesary to mock the Context, so RA collaborators are populated, but the tracer...

This comment has been minimized.

@yarel79

yarel79 Oct 31, 2016

Contributor

Tracer is used internally in SipResourceAdaptor.java in raConfigure(). If setResourceAdaptorContext used, then still some Tracer implementation is needed.

// RA#2 custom configuration
configRa2.addProperty(new Property("javax.sip.PORT", "java.lang.Integer", SIP_RA2_PORT));
configRa2.addProperty(new Property("javax.sip.STACK_NAME", "java.lang.String", raContext2.getEntityName()));

This comment has been minimized.

@jaimecasero

jaimecasero Oct 31, 2016

it seems the raContextX mock are a bit overused as well, can we really just use a literal here, and prevent mocking? We could assert later against the literal (hopefully a constant in the test class)...

I can see raContextX mocking is necessary to complete RA collaborators though...

This comment has been minimized.

@yarel79

yarel79 Oct 31, 2016

Contributor

Yes, it makes more sense to use literal instead of raContextX call, just to get entity name.

// verify RA2 stack
ClusteredSipStack stack2 = Whitebox.getInternalState(ra2, "sipStack");
Assert.assertEquals(raContext2.getEntityName(),stack2.getStackName());

This comment has been minimized.

@jaimecasero

jaimecasero Oct 31, 2016

again, do we need to mock to do this assertion?

This comment has been minimized.

@yarel79

yarel79 Oct 31, 2016

Contributor

Mock is not needed to get entity name, but sipStack field is private, therefor some API of powermock is used to help. More on that in the general remark on using Mock libraries.

// initialize internal dependencies of RA#1
Whitebox.setInternalState(ra1, "tracer", tracer1);
Whitebox.setInternalState(ra1, "raContext", raContext1);

This comment has been minimized.

@jaimecasero

jaimecasero Oct 31, 2016

shouldnt we call "setResourceAdaptorContext" using the Context mock instead?

This comment has been minimized.

@yarel79

yarel79 Oct 31, 2016

Contributor

Yes, can be used, but still raContext mock should deliver some predictable tracer mock.

@yarel79

This comment has been minimized.

Contributor

yarel79 commented Oct 31, 2016

The question is, to use mock libraries or not to use.

I guess it is a subjective. Looking purely from the perspective of a single case, it may be enough to provide DummyTracer, DummyResourceAdaptorContext, etc.

However I see some benefits of using mock libraries long term and uniformly across the project:

  • possibility to mock private fields / static methods
  • possiblilty to provide only minimal set of operations just to satisfy entry conditions for a single test

a) sipStack is a private field, therefore one would need to use some reflection API to get it, and later on to get listening points associated with the stack. This operation is facilitated by powermock toolkit (Whitebox class).

The other solution would be to expose ClusteredSipStack from SIP RA, but is this ok? I suppose there are reasons not to expose it (if opposite, then initially it would be already accessible :)).

Now, exposing getClusteredStack() brings another question. Should we, in general, have some interface (or rather interfaces hierarchy) which will expose operations only for test purposes, (some UnitTestContext / SipRAUnitTestContext etc.) ?

b) Having an interface with 20+ operations, shall I implement all these ? ("DummyTracer approach") or be lazy and provide only what is necessary? (mock one operation needed from the perspective of a given test).

c) If in need of different behaviour for another test, shall I subclass DummyTracer or to prepare another mock answer? How reusable mocks should be?

I prefer to have some generic toolkit, but again, this is my personal view.

Is there some generic approach to be applied in the project? So I could align with that or to stay with mockito+powermock ?

@deruelle

This comment has been minimized.

Member

deruelle commented Nov 7, 2016

@jaimecasero do you have to answer @yarel79 questions ?

@jaimecasero

This comment has been minimized.

jaimecasero commented Nov 7, 2016

Hi @yarel79
Is hard to suggests a proper unit test approach for SIPResourceAdaptor class, since its linked to establishing network binds on ports, so theorically speaking this is not unit testing.

As you can see, the existing tests are more focus on testing specific "wrapper" logic of different wrapper classes, as this SIPRA component is basically a wrapper of JAIN-SIP stack. For this simple wrapper UnitTest code we have not required complex mocking/stubbing techniques yet.

Having said that, I really prefer mocking versus stubbing. Again, there is no mocking approach for this particular component. For the rest of Restcomm projects we have used "mockito" lib before, so that certainly align with our current approach.

Since we are focusing the PR discussion on testing, rather than actual implementation, I would say this is ok for me. I'll wait for @SergeyLee comments to finally merge

@SergeyLee

This comment has been minimized.

Contributor

SergeyLee commented Nov 28, 2016

I will investigate this big fixing after PR #29.

@SergeyLee SergeyLee merged commit 7ccf579 into RestComm:master Dec 16, 2016

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