RestComm / jain-sip

Disclaimer: This repository is a git-svn mirror of the project found at http://java.net/projects/jsip whose original repository is developed collaboratively by the Advanced Networking Technologies Division at the National Institute of Standards and Technology (NIST) - an agency of the United States Department of Commerce and by a community of individual and enterprise contributors. TeleStax, Inc. will perform some productization work, new features experimentation branches, etc for its TelScale jSIP product that doesn't concern the community from the main repository hence this git repository.
http://www.restcomm.com/
141 stars 151 forks source link

When TCP is used, Jain-sip close socket too soon before responding 200 OK to REGISTER #129

Open xhoaluu opened 7 years ago

xhoaluu commented 7 years ago

When gov.nist.javax.sip.CACHE_SERVER_CONNECTIONS is set to false and TCP is used, jain-sip close socket too soon before responding 200 Ok to REGISTER message.

xhoaluu commented 7 years ago

When gov.nist.javax.sip.CACHE_CLIENT_CONNECTIONS is set and Tx is terminated state, Jain-sip does not close socket also.

dhstock commented 6 years ago

I have noticed a similar issue when sending an OPTIONs ping via TCP. Looking at the verbose logging jain-sip is terminating the TCP connection before it even sends out the 200 OK response. From what I can tell inside of "sendMessage" in SIPServerTransactionImpl.java the "checkStateTimers" is running before we even try to send the message. Since this is TCP "checkStateTimers" is setting timerJ to 0 and immediately terminating the TCP connection. I was able to work around the issue by moving the "checkStateTimers" inside the try block(after the sendResponse).

Does this seem like a valid fix? From my understanding of the RFC timerJ being set to 0 for TCP is to ensure we don't re-transmit beyond 1 attempt.

Old Code:

            if(!checkStateTimers(statusCode)) {
                if (logger.isLoggingEnabled(LogWriter.TRACE_DEBUG)) {
                    logger.logDebug("checkStateTimers returned false -- not sending message");
                }
                return;
            }

            try {
                // Send the message to the client.
                // Record the last message sent out.
                if (logger.isLoggingEnabled(LogWriter.TRACE_DEBUG)) {
                    logger.logDebug(
                            "sendMessage : tx = " + this + " getState = " + this.getState());
                }
                lastResponse = transactionResponse;
                lastResponseStatusCode = transactionResponse.getStatusCode();

                this.sendResponse(transactionResponse);

New Code:

try {
                // Send the message to the client.
                // Record the last message sent out.
                if (logger.isLoggingEnabled(LogWriter.TRACE_DEBUG)) {
                    logger.logDebug(
                            "sendMessage : tx = " + this + " getState = " + this.getState());
                }
                lastResponse = transactionResponse;
                lastResponseStatusCode = transactionResponse.getStatusCode();

                this.sendResponse(transactionResponse);

                if(!checkStateTimers(statusCode)) {
                    if (logger.isLoggingEnabled(LogWriter.TRACE_DEBUG)) {
                    logger.logDebug("checkStateTimers returned false -- not sending message");
                }
                return;
            }

The log message above would not be 100% correct since we would have sent 1 message.