Adding message (de)serializing for connection-oriented SCCP messages

[vmykhailiuk]

Created interfaces for various SCCP connection-oriented messages and their implementation with (de)serializing support.
Going to create pull request (major patch) with corresponding changes.

[vetss]

Hello @vmykhailiuk

thanks for your contribution. I added you PR to master branch #243.

My comments:

1. better to add some comments into interfaces for what message is implemented (for example in https://github.com/RestComm/jss7/blob/master/sccp/sccp-api/src/main/java/org/mobicents/protocols/ss7/sccp/message/SccpConnAkMessage.java)
   We can mart that this is "Data acknowledgement (AK)" message like we added comments for SccpDataMessage for example, can you please that comments ?

2. Your approach of implmenting of MessageFactoryImpl is incorrect. We need two methods types there:
   a) public SccpMessageImpl createMessage(int type, int opc, int dpc, int sls, InputStream in, final SccpProtocolVersion sccpProtocolVersion, int networkId) - for parsing of incoming messages
   b) methods like public SccpDataMessage createDataMessageClass0(), public SccpDataMessage createDataMessageClass1(), createNoticeMessage() that are used for creating of outgoing messages.

We need to create a set of methods for new connected-oriented messages (and add them also into MessageFactory interface). We may not have a method "SccpMessageImpl createMessage(int type, int opc, int dpc, int sls, int networkId)". Please update the class / interface / tests for this case.

3. I have not checked encoding / decoding implementations details. I beleive you have soem trace data from live traces and reused them for unit tests. If you have any doubts for implementing please let me know.

[vetss]

PS: a list of added commits I will put into an initail issues for this topic:
#239

[vmykhailiuk]

Hi @vetss
Thanks for your comments. I have questions:

2. Currently MessageFactory interface has following methods:

SccpDataMessage createDataMessageClass0(SccpAddress calledParty, SccpAddress callingParty, byte[] data, int localSsn, boolean returnMessageOnError, HopCounter hopCounter, Importance importance);

SccpDataMessage createDataMessageClass1(SccpAddress calledParty, SccpAddress callingParty, byte[] data, int sls, int localSsn, boolean returnMessageOnError, HopCounter hopCounter, Importance importance);

// SccpNoticeMessage createNoticeMessage(ReturnCause returnCause, int outgoingSls, SccpAddress calledParty, SccpAddress callingParty, byte[] data, HopCounter hopCounter, Importance importance);

What methods should be added to MessageFactory and MessageFactoryImpl:

public SccpMessageImpl createMessageClass2(int type, int sls, int localSsn) {
    SccpMessageImpl msg = null;
    switch (type) {
        case SccpMessage.MESSAGE_TYPE_CR:
            msg = new SccpConnCrMessageImpl(this.sccpStackImpl.getMaxDataMessage(), sls, localSsn);
            break;
        // other message types of protocol class 2
    }
    return msg;
}

public SccpMessageImpl createMessageClass3(int type, int sls, int localSsn) {
    SccpMessageImpl msg = null;
    switch (type) {
        case SccpMessage.MESSAGE_TYPE_CR:
            msg = new SccpConnCrMessageImpl(this.sccpStackImpl.getMaxDataMessage(), sls, localSsn);
            break;
        // other message types of protocol class 3
    }
    return msg;
}

or

public SccpConnAkMessage createAkMessageClass3(int sls, int localSsn, LocalReference reference, ReceiveSequenceNumber receiveSequenceNumber, Credit credit) {
}

// other per-message type factory methods
    
public SccpConnRlsdMessage createRlsdMessageClass23(int sls, int localSsn, LocalReference destinationLocalReferenceNumber, LocalReference sourceLocalReferenceNumber,  . . .) {
}

?

3. Haven’t used trace data from live traces yet. There will be additional compatibility testing later by using third-party SS7 stack and at that point I will be able to actually capture live traces and potentially reuse them in tests.

[vetss]

Hello @vmykhailiuk

What methods should be added to MessageFactory and MessageFactoryImpl

createDataMessageClass1() and createDataMessageClass2() - this is messages for creating DATA messages from outside of SCCP stack == for a SCCP stack user.

For connected-oriented case we need to think which messages are needed for a SCCP stack user. This depends on a future management interface of SCCP stack. If for example for connection creating a user will invoke an interface method like "establishConnection()" and an argument will be "XXXMessage" than we need to create the concrete method in factory to create a concrete primitive.

We may have a separate discussion of management interfaces.

As for live traces, I got you. Better to update unit tests when you have live traces to be sure.

[vmykhailiuk]

Hi @vetss

Designed the following API for connection-oriented protocol classes.

Going to add such methods to MessageFactory:

SccpConnCrMessage createConnectMessageClass2(SccpAddress calledAddress, SccpAddress callingAddress, byte[] data, Importance importance);

SccpConnCrMessage createConnectMessageClass3(SccpAddress calledAddress, SccpAddress callingAddress, Credit credit, byte[] data, Importance importance);

SccpConnDt1Message createDataMessageClass2(LocalReference destinationLocalReferenceNumber, SegmentingReassembling segmentingReassembling, byte[] data);

SccpConnDt2Message createDataMessageClass3(LocalReference destinationLocalReferenceNumber, SequencingSegmenting sequencingSegmenting, byte[] data);

Creating interfaces SccpConnection, SccpClass2Connection, SccpClass3Connection:

public interface SccpConnection {
    int getSls();

    // source local reference
    LocalReference getSlr();

    // destination local reference
    LocalReference getDlr();

    void disconnect(ReleaseCause reason, byte[] data);

    // not available after disconnect
    boolean isAvailable();
}

public interface SccpClass2Connection extends SccpConnection {

    void send(SccpConnDt1Message message);
}

public interface SccpClass3Connection {
    Credit getCredit();

    void send(SccpConnDt2Message message);

    void reset(ResetCause reason);
}

Adding methods to SccpListener:

// can call conn.confirm() or conn.disconnect(...) with refuse reason and data
void onConnectIndication(SccpPendingConnection conn, SccpAddress calledAddress, SccpAddress callingAddress, ProtocolClass clazz, Credit credit, byte[] data, Importance importance);

void onConnectConfirm(SccpConnection conn);

void onDisconnectIndication(SccpConnection conn, ReleaseCause reason, byte[] data);

void onResetIndication(SccpConnection conn, ResetCause reason);

void onResetConfirm(SccpConnection conn);

SccpPendingConnection interface is:

public interface SccpPendingConnection {
    // can be used only in SccpListener.onConnect
    void disconnect(RefusalCause reason, byte[] data);

    void confirm();
}

Adding method to SccpProvider:

// intiates establishment of connection
void establishConnection(SccpConnCrMessage message) throws IOException;

Adding extra method to RefusalCause, ReleaseCause:

DisconnectOriginator getOriginator();

where DisconnectOriginator is enum:

public enum DisconnectOriginator {
    NSU, NSP, RESERVED, UNDEFINED;
    //. . .
}

Please comment could I continue implementing it or something should be changed in API.

[vetss]

Hello @vmykhailiuk

thanks for sharing your interfaces data, I have some comments for your solution:

1. MessageFactory.createDataMessageClass2() , createDataMessageClass3() :

• you provide LocalReference in these methods. This parameter is stored in SccpConnection class and can be easily assigned there when we invoke SccpConnection.send() method by SCCP stack (by invoking of SccpConnDt1Message.setLocal/RemoteReferense ). I suggest you to remove LocalReference from these methods.
• SequencingSegmenting in terms of Sequencing side - I think SCCP stack is also responsible for this part (assigning of a sequense number). So same recommend - remove it fro a factory method.
• SegmentingReassembling - this just a boolean parameter (if this is a last segment or not). I see here two approaches:
  a) SCCP stack is responsible for segmenting / reassembling (say we will push in createDataMessageClass2 the whole data in data[] and SCCP stack will make segmenting / reassembling). And we do not need SegmentingReassembling parameter in this case.
  b) SCCP user is responsible for segmenting / reassembling and it is enoupgh a parameter like "boolean lstSegment" for both factory methods.
  "a)" looks me more convinient for usage, but if you do not use long size data in your current tasks we can stick in "b)"

2. SccpConnection / SccpPendingConnection :

• I see more reasonable to have s single interface for all connections lifecycles and also State getState() method where we can get states: CR_SENT(we have not yet received CC), CR_RECEIVED (we have not yet sent CC), ESTABLISHED, CLOSED. This will allow us for having / storing a single connection object inside a SCCP stack for whole life cycle.
• SccpClass2Connection / SccpClass3Connection - we may have these two interfaces or may have single interface SccpConnection with method getConnectionClass(), this option is for your choice.
• LocalReferense.getSlr() / LocalReferense.getDlr() we need instead of "source" / "destination" have "local" / "remote". LocalReferense.getLocalRefernce() will be a key for keeping of connections in SCCP stack. LocalReferense.getRemoteRefernce() - is LocalReferense for a remote peer.

3. When we have a single SccpConnection class (or SccpClass2Connection / SccpClass3Connection) we need to be able to get a SccpConnection from SCCP stack by its local referense.
   SccpConnection SccpProvider getConnection(LocalReference localReference)

4. SccpListener does not have any methods for received data like onData()

5. Connection confirm (CC) message MAY have Responding address parameter ("new Called Party Address"). You may be need to add this parameter into confirm(). It is not clear for Credit , Importance field, may a a node change it at CC sending step. Probably we do not need / may not do it.

6. We need to think of some error cases and announdsing of a SCCP user for them (in SccpListener)

• Connection establishing timeout
• Flow control / (Flow control window) case. If a SCCP user wants to send a set of messages and Flow control window is full (sccp class 3 case), than a stack (from my point of view) must accept and cache in some local buffer such messages. In such approach we may have overloading / message dropping cases.
  We may need to think of error cases and how to cover them.

7. Will you have an implementation of a "Relay node" which will be used for a SCCp message transit ? I do not see any methods for this case.

If you need to discuss anything else, please let me know.

[vmykhailiuk]

Hi @vetss

Thanks for your comments and quick response.
Have following notes about pull request #244.

1. Only two methods were added to MessageFactory: createConnectMessageClass2 and createConnectMessageClass3 which will be used with SccpProvider's method establishConnection(SccpConnCrMessage message).

2. SccpConnCrMessageImpl is initialized inside MessageFactoryImpl methods using setters to avoid having two different constructors for different protocol classes.

3. Other messages will be created inside connections method and so user doesn't need additional methods for them in MessageFactory. For example, SccpConnection will create either SccpConnDt1Message or SccpConnDt2Message in connection.send(byte[] data) method depending on connections protocol class.

[vetss]

Hello @vmykhailiuk

your PR #244 looks good for me, I have added it into a amaster branch.

Looks forward to a core level implmentation.

[vmykhailiuk]

Hi @vetss

Thanks for reviewing my pull request.

Have following notes about pull request #246.

BaseSccpListener was created to avoid declaring connection-oriented listener methods in test listeners used by non connection-oriented tests.
ConnectionTest has test cases for connection establishment and release.
SccpRoutingControl has a number of methods, such as routeMssgFromMtp(…), sendMessageToMtp(…) for handling three types of cases:

• protocol classes 0, 1 messages;
• protocol classes 2, 3 non-CR messages;
• CR message.

Each case requires different messages sent in case of error (correspondingly):

• Notice message with return cause;
• RLSD message with release cause;
• CREF message with refused cause.

There is some duplication of routing code methods but they have error handling differences.

SccpConnCrMessageImpl doesn’t inherit SccpAddressedMessageImpl because that won’t allow using the same method for handling all addressed messages anyway (it would be too large and would consist from two similar parts for handling CR and non-CR messages routing) and also because it doesn’t need getReturnMessageOnError, clearReturnMessageOnError methods.

[vmykhailiuk]

Segmenting / reassembling, flow control / (flow control window) case, message transit are planned to be implemented later.

[vetss]

Hello @vmykhailiuk

I can treat the update as draft one because not all staff is implemented and therefor I do not add it into a main repo. I have not checked also tests.

My comments for the current update:

1. BaseSccpConnectionImpl.executor - I suggest to move it into a Stack level as a single executor. Current implementation wants to create several executor threads per a connection (server resource will be wasted)

2. I have not found a reason why do we need to keep stack.referenceNumbersUsed - can not we get info of a connection referense from "stack.connections" ?

3. TCAP stack has now compilation errors - we need to update it before we can accept your update

4. may be rename "getConnectionNumber()" -> "getConnectionsNumber()" (else it sounds as a number of a concrete connection)

5. introduced new parameters into stack are present in store() method, but I do not see them in "load()"

6. conn.setState(SccpConnectionState.ESTABLISHED); - when you do it you do not check the previous state. It mat\y be wrong may be closed or olready estanlished. You need to check all "conn.setState(...)" cases.

7. one important part that I need to discuss separately - the style of how you implement the routing part. We have now (after your implementation) several parts for processing
   a) connectionless messages,
   b) CR message
   c) other connection-oriented messages
   for example
   routeMssgFromMtp(SccpAddressedMessageImpl msg)
   routeMssgFromMtp(SccpConnMessage msg)
   routeMssgFromMtp(SccpConnCrMessageImpl msg)
   routeMssgFromSccpUser(SccpAddressedMessageImpl msg)
   routeMssgFromSccpUser(SccpConnMessage msg)
   send(SccpMessageImpl message)
   sendCr(SccpMessageImpl message)
   send(SccpConnMessage connMessage)
   etc

Routing for connectionless messages and a CR message looks for me more or less similar. But now after your implementation we have doubled code methods with code that is more or less same that will make code maintananse more difficult in future.
In this case I see we can inherit SccpConnCrMessageImpl from SccpAddressedMessageImpl so we will be able to resuse this scenario.

Routing of connection-oriented messages (including CC) is much different as compare with a) and b) and we can and much simplier. We do not need to make any transaltion, any SSN / address analisys etc. All routing info must be already connected into a Connection object and simply reused. I see we can simplify this part by usage of may be two methods like
routeConnMessageFromMtp()
routeConnMessageFromSccpUser()

Please provide if agree for my suggestions for "7)" or give me your doubts why it is bad / unreachable / very difficult. We can make a separate discussion for this topic.

PS: now is an important step when we have to incorparate new functionality (connection-oriented messageflow) into a core level. We need to make it carefully to avoid of current fuctionality break.
I suggest to finish of case "7)" and other cases so then we will be able to accept your PR and continue of implementation for missed parts.

[vetss]

Hello @vmykhailiuk

I checked you two last updates from PR and they looks good for me. Sorry for some delays in processing.

1. we need to fix case "7)" and then I will be able to recheck code. Please let me know when it is ready.
2. other stacks has still compilation errors (check of unit test part please)

This is a separate branch that contains all updtes from you :
https://github.com/RestComm/jss7/tree/sccp-co
The code is not added into a master banch till we fix all issues.

[vmykhailiuk]

Inherited SccpConnCrMessageImpl from SccpAddressedMessageImpl and reused SccpAddressedMessageImpl routing scenarios as per 7).

[vetss]

Hello @vmykhailiuk

I have added a commit from your PR #251 into "sccp-co" branch. SccpRoutingControl looks much better now. And I see not functionality is already fuly implemented.

My comments:

1. I think that in SccpListener.onConnectIndication(...) we can accept or reject an incoming connection request:

• SccpConnection.confirm(SccpAddress respondingAddress) - accept
• SccpConnection.disconnect(RefusalCause reason, byte[] data) - reject a connection
  If yes - better to rename this "disconnect()" -> "refuse()"

2. code thread locking per a connection. Some connection operations must be atomic. For example changing of a connection state + sending a message as a response must be atomic. It may be live cases when a message is coming from a peer from one side and from other side a sccp user may send some message to a peer and it will be concurrent invoking from different threads. I can see "BaseSccpConnectionImpl.connectionLock" object but a usage of it is strange. It is used only inside timers and may cover some cases like "this.future = stack.timerExecutors.schedule(this, delay, TimeUnit.MILLISECONDS);" that are connection safe itself. We have also "synchronized (this)" in "setState(SccpConnectionState state)" that is a different locking object. I suggest to use a single reentrance lock (the example is TCAP Dialog). We must not include inside synching area a process of message sending to a peer because it is a time consuming operation.
   Also we are going to change of threading model (see Thread affinity style for TCAP dialogs processing #196). This is for a future, this is just for your info.

3. for what do we have SccpConnection.isAvailable() ? true when a connection is already closed (or may be not yet connected) ? This is not implemented

4. BaseSccpConnectionImpl.setState(SccpConnectionState state) - may it be a switch from states like NEW, CR_SENT etc to CLOSED ? I think yes, but it is not present in a list of available operations.

5. SCCP stack has a special previewMode that is needed when SCCP stack does not play a role of SS7 peer but just listerns a passing traffic and provide it into a SCCP listener. This demands extra efforts, I just explain what is it (not very needed to include it into a first solution).

6. A reminder - we need to have WARN level messages for error cases (where we must explai what happens). It may be a an error message from a peer or no possibility to process something because of local reasons, bad dialog states etc. For a success operations (connection establishing, data messageflow etc) we need to have a DEBUG level logging.

7. I can see for connected oriented messages:
   SccpListener listener = conn.getListener();
   if (listener == null) { ... }
   I think better to check of a listner before a connection establishing (and reject of connection in bad SSN case).

8. let's split SccpRoutingControl.route(SccpMessage message) - let's split this method into two (for addressed / connected oriented messages). This is not a bug, just for more clear code.

9. SccpRoutingControl line 1309 - "if (ans != null) {" - ans is always !=null in this case.

10. SccpRoutingControl.sendSccpErrorConn(SccpConnMessage msg, ReleaseCauseValue cause). I believe the method is for some error case when we need to drop a connection. I see that you send a meesage to a peer but no actions with a local connection. If I am not right for a behavior please explain a desired behaviour.
    If we just want to close a connection - may be we can reuse a single method with functionality in every error cases - a method like SccpConnection.disconnect(.....)

11. We have a congestion control for incoming and outgoing messages in SCCP stack. We need to care it. Check Congestion control #26 . SCCP connection may play a same role as TCAP stack from congestion point of view. For incoming messages SCCP must ask for a upper stack for its congestion (do not underatns how) and rejects new connections / all messages depending on a congestion level. For outgoing messages BaseSccpConnectionImpl.sendConn(SccpConnMessage connMessage) - we need to set a proper value for "msgImportance" value:

• connection establish - 3
• data - 5
• other messages - 8
  (the idea is to drop firstly connection establish messages, then data and keep of maintanense traffic)

12. BaseSccpConnectionImpl.sendConn(SccpConnMessage connMessage) - LongMessageRule / LongMessageRuleType is always "LONG_MESSAGE_FORBBIDEN" for connected oriented messages. We have other options only for connectionless service, let's simplify code.

13. BaseSccpConnectionImpl.disconnect(ReleaseCause reason, byte[] data) runs relProcess timer that if no answer raise disconnect() again. Is it correct ?

I can not check the whole code and may be mistaken for some raised issues, we can discuss.

[vmykhailiuk]

@vetss
Created pull request #253 with SCCP protocol class 3 flow control support. Going to update code according to all comments #242 (comment) except for preview mode and congestion control in next pull request (i. e. not in #253 ).

[vetss]

Hello @vmykhailiuk

thanks for your update, I see a big progress. Please check my comments here:

1. We have again TCAP compilation error after update updateing of SCCP interfaces

2. I have forgotten one important thing from the very beginning. Each java file must contain a file header with a license. See details in https://docs.google.com/document/d/1RZz2nd2ivCK_rg1vKX9ansgNF6NpK_PZl81GxZ2MSnM/edit#heading=h.iayg4mgzyzs8 . An example of a java file with the actual header - https://github.com/RestComm/ussdgateway/blob/master/core/domain/src/main/java/org/mobicents/ussdgateway/UssdManagement.java.

3. I have some doubts if credit and receive/sentFlowControlWindowStart/End are proper calculated. Examples:

• FlowControlWindows.setCredit(int credit) - send / receive credit may be different. For example if a local side is overloaded (incoming congestion control) then we can configure a credit to a peer to 0 to stop of sending of messages from a peer. But our local credit for sending may be > 0. Also We configure receive/sendFlowControlWindowEnd here like possible values for "send sequence number P(S)" are 0-"credit" althow (IMHO) they must be "modulo 128" == 0-127 (no matter what is a current credit value)
• FlowControlWindows.handleSequenceNumbers(...) - "sendSequenceNumber <= receiveFlowControlWindowEnd" - sendSequenceNumber may be 120 for example and receiveFlowControlWindowEnd 20 for example (P(S) is cycling), so I mean we may be calculate the condition in uproper way.
• to be sure we need to prepare a special unit test for sending / receiving SequenceNumber managing. We need at least make tests till SequenceNumber cycling (moving over value 127 to 0), testing with big credit (127), low credit (for example 1-2-3) (this test must include a case when a window is exasted because of a peer does not sent intime a response and then sends a response that will lead of resending of pending because of window lack), a case when a peer is overloaded and configure credit==0 and further change the value to a some credit >0, a case of bad messageflow that leads or reset and then reset procedure and resending of pending messages, may be other tests. We need to test different cases very carefully in lab !!

4. send(byte[] data) - you use "Thread.sleep(SLEEP_DELAY);" for waiting till a connection becomes established or a free window space. It is a bad practice (because of thread bloacking). Much better is to put pending messages into some queue and when we have a window space we take messages fromk queue and send them. We need here also some timer so will can termitate a connetion if no success traffic for much time (message waiting for too much time or tooo many messages in the queue). Also we need to queue also sent but unconfirmed messages (no confirmation that a peer has received it) in order to resend them after a reset procedure.

5. You use "synchronized" definition for several methods. Generally we need of synchyning to avoid collisions. But based on a method restrictions makes sense if this method contains some atomic operation for example assigning of new id from a row of possible ids. In this case the method operates with some staff that is processed only by the method. Else may be we need to have some synch blocks with a single object is a connection object for example. In your case I can see that if a connection receives DT2 and AK messages at a same time and for example a user want to send a message then all these will be processed at the same time. We can make a separate discussion for the best solution for this topic (synching) to minimize synching areas and make 100% sure.

6. I do not see how a user can mange / configure a "credit" for outgoing / incoming messages (if we want to increase / decrease it).

I will add you commit into a "sccp-co" branch and we need to fix issues to finish the work.

[vmykhailiuk]

@vetss
Added segmentation support.
Updated according to comments listed in #242 (comment) except preview mode, congestion control, atomic connection operations, warnings for error cases. Will add warnings and atomicity in next pull request.

As for

BaseSccpConnectionImpl.disconnect(ReleaseCause reason, byte[] data) runs relProcess timer that if no answer raise disconnect() again. Is it correct ?

It's correct. In Q.714

3.3.3 Actions at an end node initiating connection release
3.3.3.1 Initial actions
When a connection release is initiated at an end node of a signalling connection, by the SCCP user
invoking an N-DISCONNECT request primitive or by the SCCP itself, the following actions are
performed at the initiating node:

1. An RLSD message is transferred on the connection section.
2. A release timer T(rel) is started.
3. If the release was initiated by the SCCP, then an N-DISCONNECT indication primitive is
   invoked.
4. The inactivity control timers, T(ias) and T(iar), if still running, are stopped.

3.3.3.2 Subsequent actions
The following actions are performed at the originating node on a connection section for which an
RLSD message has been previously transferred:
...
2) When the release timer expires, an RLSD message is transferred on the connection section.
The sending of the RLSD message is repeated periodically.

[vetss]

Hello @vmykhailiuk

I have added your PR #255 into sccp-co branch.

Commants:

1. testSendSegmentedDataProtocolClass3() - (may be other tests like this) - we need to check if an incoming message content is the same as was sent
2. We have not uimplemented intactivityTest - this is mandatory I think
3. We need to add a set of unit tests for variaty of cases espesially that use some difficult algo like:

• testing of different timeouts (for example CR sent etc)
• window size (credit) changing, waiting for free window space when message sending, low / big window size, a whole cycle of P(S) values (0-127-0-...), bad cases and reset, etc

[vmykhailiuk]

Hi @vetss
Changed credit and sequence number calculation support, fixed TCAP compilation error. Going to provide more tests and locking changes in next pull request.

[vetss]

Hello @vmykhailiuk

I have checked your PR and a commit "SCCP credit fixes". I hope this commit contains your update for changing of message sequensing and I have not missed anything. The second commit was for TCAP stack error fix.

I have checked your update for changing of message sequensing. I still have doubts of we make it in a proper way.

1. we have methods getAllowedReceiveFlowControlNumbers() and getAllowedSendFlowControlNumbers() that prepare a list of allowed values and than check if a number belongs to it. It consumes computing time and memory for the list. I suggest just to use "<", ">" etc operations for checking of if a number belongs to a range without a list creating. This is a productivity issue.
2. handleSequenceNumbers() - receiveSequenceNumber processing - may be an error processing when for example lastReceiveSequenceNumberReceived==125 and nextSendSequenceNumber==2
3. I see that you send SccpConnAkMessageImpl message only when a Window of a remote side is exhausted. This thing is not described in a manual and no clear algo. We need to prepare an algo when we will send an AK message when no data messages for sending. Else a peer may wait for a confirmation for long time. We can discuss it.

We definitely need a set of unit tests to finish of sequensing. Please finish this step.

PS: I added 2 commits from your PR 256 to a master branch

[vetss]

Hello @vmykhailiuk

can we finish of fixing of issues form my comments in short time ?

[vmykhailiuk]

@vetss
Created pull request #265 with SCCP message transit support.
Also:

• changed how locking is performed for connection, now using single reentrance lock (as in TCAP Dialog org.mobicents.protocols.ss7.tcap.DialogImpl where send and receive methods "public void send(...)" and "void processUni(...)" method bodies are wrapped in dialogLock lock/unlock);
• added message transfer queue to avoid blocking when sending data messages (however, message types such as RLSD, RLC, RSR, RSC are sent immediately and synchronously to avoid delaying connection release and reset;
• added more logging for connection establishing, reset, etc using WARN level for error cases;
• added check that there is a listener when establishing connection;
• updated to set msgImportance for messages according to your comment;
• changed how comparison is performed for message flow sequence numbers;
• updated SccpListener onConnectConfirm to provide data when it's present;
• added more tests.

[vetss]

Hello @vmykhailiuk

I see we have a big progress. Thanks for your work.

1. I managed to have only a short look at your update

• I added one fix (one of unit tests failed at my side)
• FlowControlWindow - why do we make "sych" of many methods there ? I am trying to understand your logic
• I see SccpStack is with scheduler now. We need to update jboss 5 script (if you have not it done)
• you have many methods "toString()" without named of classes. So you for example print a value of a parameter (s) but not clear what class is it. Please fix it
• I am thinking to push some connection-oriented only methods to special packages to make code more clear. It may be package name like "sccp.impl.co"

2. I pushed code into "sccp-co" branch. Please SYCH it at your side before next updates. This branch contains also many updates from master branch that I merged.

I need to make some extra checking of your code before we can merge it into master branch.

[vetss]

Hello @vmykhailiuk

please check my next comments after checking of the implementation

1. FlowControlWindow methods like public synchronized void setSendCredit(int credit) are synchronized. IMO we need to check if all methods for changing of a connection state is inside connectionLock.lock(); and remove of synchronized for such methods. If you have some explanation that we need such synchronized behaviour please exaplain it.
   We need aldo check if all connection processing is locked under a connection level connectionLock.lock();

2. SccpListener SccpConnectionBaseImpl.listener - it is stored in a connection. The most probably we need to avoid of keeping listener in SccpConnectionBaseImpl because it may be changed / removed by a SCCP user. And we need to check if it is !=null before of usage

3. SCCPStack: I see several new parameters were added like connEstTimerDelay. They are stored into a config xml file but no management by CLI or GUI (do we have an explanation in a manual ?). We need to add CLI / GUI support (if not yet added) and manual descripton (if not added).

4. protected FastMap<Integer, SccpConnectionImpl> connections = new FastMap<Integer, SccpConnectionImpl>(); - collection is not thread-safe, setters are thread safe (it is ok), getters not that may give collisions accidently. Better to add locking to getters or use of a thread-safe collection.

5. Specification Q.714 - ANNEX B has several tables with a description what to do in case of errors (bad messages are received, wrong connection state, etc). Check tables like B.1/Q.714 - B.5/Q.714.
   Have you checked this staff and implemented of a proper behaviour ? If not - better to recheck it and make needed updates in processing of error cases.

6. SccpRoutingControl.sendConn(...) - what happens when the stack can not send a message to MTP (SS7 channel down for example) ? We need to annonse a local user if a message is local originated (I have not found it) and send a message to a SS7 peer (it looks like it is implement but a message will not be sent because of channel is down (that may lead a loop). I think we need to differentiate messages from local SCCP user / from MTP when we are generating errors and depending on it decide if we need to announse a local user / remote peer / both.

7. unit tests:

• I have not found a sequensing test that tests message exchange for many messages which cover all sequence number from 1 to max and again from 1. We can send messages from node A to B and wait for responses with message receive confirmations.
• error cases testing like no local connection for an incoming message, can not send mesage to MTP (channel down for example), bad sequense number received / bad sequesnce confirmation received, no receive confirmation from a peer, connection reset case (may be as a part of previouse described case), bad message segemted message received (for example a missed segment), may be other cases that are not covered

8. we do not have a perfomance test. For nonnectionless messageflow we reuse https://github.com/RestComm/jss7/tree/master/map/load performance test, we can get it as an example. For connection-oriented message flow I see a test as two parts (Client / Server), a Client will open connections, send several test messages and close connections in a concurrent manner.

9. We do not have a functional test. We have a tool for it SS7 Simulator - https://github.com/RestComm/jss7/tree/master/tools/simulator . I do not know it this functionality in scope of implementation. If yes we can extend SS7 Simulator functionality for this.

[vmykhailiuk]

Hi @vetss
Created pull request #272 with SCCP protocol classes 2 and 3 changes. Have following changes:

• FlowControlWindow class was renamed to SccpFlowControl and changed to implement specifications more fully (for example, previously after receiving AK message sequence number P(S) was starting over from 0 and now it keeps incrementing until reaching maximum value 127);
• SccpFlowControl has no locking now and relies on connection locking in methods which use it;
• connection doen't store copy of listener reference anymore;
• added CLI options for new SCCP stack parameters, documented that;
• documentation for SCCP connection-oriented protocol classes with code example;
• now using FastMap<Integer, SccpConnectionImpl>().shared() for connections collection thread-safety;
• sequencing and error cases tests;
• updated jboss script;
• updated toString() methods to show class names;
• tests for credit changes by user;
• tests for waiting for free window space, low / big window sizes, overloading (i. e. setting receive credit to 0 temporarily).

[vetss]

Hello @vmykhailiuk

I have made checking and testing of whole code. Code is now here:
https://github.com/RestComm/jss7/tree/sccp-co
It includes commits from PR #272 and my extra commits with some bug fixes and testing suits some updates.

What made:

• added "toString()" to many message implememntation classes
• I made some refactoring of SccpRoutingControl: I extracted multiple reused code into separate methods (processIncCR() and processCoMessages()) and updates some non-proper implemented functionality including of SSN processing for that you mentioned in the ticket. Please check this work part if you have any doubts or updates
• I do not think we need put crMes.setLocalReferenseNumber() before a connection establishing. It may confuse some customer that wants to create their own code based on unit tests. Let's put min needed sode in unit tests (this is for future)
• there were no unit tests for the routing based on GT. I added it. It led some code fixes to force it work
• I removed of deleting all files from current folder at unit tests (this functionality killed me all files from a root folder under Eclipse). I made other approach to avoid of reusage of configs betweet tests, hope it works well enouph
• I made in tests different SSNs at different stacks for testing of proper SSN processing

Comments for possible missing parts:

• do we have unit tests sccp user1 -- stack1 -- sccp user 2 (this means both users are in the node and no traffic between sccp stacks)? If not it makes sense to add it.
• DT2 message is processed at a transit node for coupling mode including of degmenting. Do we really need it ? We can discuss the staff.
• possible bug on generating of AK algo that may lead of one part is blocked with waiting of a peer confirmation (discussed in skype)
• we do not have a transition in setState() for ESTABLISHED_SEND_WINDOW_EXHAUSTED -> DISCONNECT_INITIATED (and let's check transition to other cases like reset initiated). We need to add because this raises an exception when we want to disconnect
• SeqiesnseNumber.increment() method does not really increment SeqiesnseNumber itself but creates a next SeqiesnseNumber. Maybe we need to rename to SeqiesnseNumber.nextNumber() ?
• Sometimes we do not analyze all parameters after unit tests. For example after testing of message exchange it makes sense to check how many messages were received. Let's reuse this checking as much as possible for make testing more complete
• I have not found how the parameter SccpFlowControl.preemptiveAk is configurable. Can you comment this please (may be need it)
• SccpFlowControlge.getUpperEdge() - this method may be does not take into account that value may become > 127 and became 0.

PS: let's avoid of bit code refactoring from now to minimize of possible breaks for what we have already tested and make kust bug fixing, testing.

[vetss]

Hello @vmykhailiuk

I have added your 3 commits and my commit for a field changing into sccp-co branch and I finished of testing and code checking.

What I see which unit tests are missed. I tried to imaging some life cases (if any of this tests are already implemented then sorry for mentioning them, please let me know where to find them):

1. do we have unit tests sccp user1 -- stack1 -- sccp user 2 (this means both users are in the node and no traffic between sccp stacks)? If not it makes sense to add it.
2. Inactivity test message -> no response from a peer (this is simulated by a test that a peer is dead) -> termination of a local connection
3. relProcess and repeatRelProcess timers. We have sent RLSD and no response from a peer (simulatoing of it). We need to check how all these timers are worked (checing of it)
4. CREF from a peer - processing of it
5. data message reassemblingFailure. Simulating that a 1) bad segment number is received and 2) no next segment is received (timer must be triggered - reassemblyProcess)

This is a list for what I have questions/doubts for implementation:
a) Before your last commit the test ConnectionFlowControlTest.testBigCreditTwoDirection() failed in about 20-40% of testing. Now it goes much better, but I still have failures (for not messages are sent) at about 5% or even less tests. So the most probably we have another reason for test failure. Can you investigate it more for a root reason for such failures.
b) I can see a checking of msg.getSourceLocalReferenseNumber() for RLSD, but I can not see it for CC, RLC, IT. I think we need them
c) I have some doubts for segmanting

• we do not make reset of lastReceiveSequenceNumberReceived and have a risk that next messages after reset will not fit to checking
• I have not found invoking of SccpFlowControl.reinitialize() after sending / receiving of RSR / RSC
• inputWindow.setLowerEdge(sendSequenceNumberExpectedAtInput); is made in several initializeMessageNumbering() methods. Will not it be more clear if we pove it to checkInputMessageNumbering() where this value is really changed ?

PS: I have not checked how well the implementation fits to a way we process error cases (proper local actions, proper sent error codes, full checking of incoming message content etc).

[vmykhailiuk]

Hi @vetss
#281 changes (items correspond to items of your latest comment, previous comment items were already implemented):

1. Added tests for case when both users are on the same stack.
2. Inactivity test already existed (org.mobicents.protocols.ss7.sccp.impl.ConnectionTimersTest#testInactivity).
3. Added relProcess and repeatRelProcess timers tests.
4. CREF test (org.mobicents.protocols.ss7.sccp.impl.ConnectionTest#testConnectionRefuse).
5. Omitted as discussed.

Other:
a) testBigCreditTwoDirection is fixed.

b) Updated implementation and added tests for incorrect msg.getSourceLocalReferenseNumber() :

• getting ERR for incorrect RLSD, RSR, RSC, IT;
• discarding RLC for incorrect RLC;
• no such tests for CC as per specification (remote LRN isn't known before CC so can't compare it).

c) added re-initialization as discussed.

[vetss]

Hello @vmykhailiuk

I added all code into jss7 master branch. I am closing this issue now as solved. If any bug are found we can open separate issues for them.