Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avoid sending double CRLF keep alive on TCP socket shutdown #172

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Avoid sending double CRLF keep alive on TCP socket shutdown #172

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4bca5e
avoid sending CRLF keep alive on TCP socket shutdown
fre42 Jan 30, 2018
9ecbe44
Fixed CLRF typo to correct CRLF
fre42 Feb 1, 2018
eba48c7
Merge remote-tracking branch 'upstream/master'
fre42 Feb 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/gov/nist/javax/sip/parser/NioPipelineParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,9 +219,9 @@ private boolean readMessageSipHeaderLines(InputStream inputStream, boolean isPre
crlfReceived = false;

try {
sipMessageListener.sendSingleCLRF();
sipMessageListener.sendSingleCRLF();
} catch (Exception e) {
logger.logError("A problem occured while trying to send a single CLRF in response to a double CLRF", e);
logger.logError("A problem occured while trying to send a single CRLF in response to a double CRLF", e);
}
} else {
crlfReceived = true;
Expand Down
4 changes: 2 additions & 2 deletions src/gov/nist/javax/sip/parser/PipelinedMsgParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,9 +366,9 @@ public void run() {
isPreviousLineCRLF = false;

try {
sipMessageListener.sendSingleCLRF();
sipMessageListener.sendSingleCRLF();
} catch (Exception e) {
logger.logError("A problem occured while trying to send a single CLRF in response to a double CLRF", e);
logger.logError("A problem occured while trying to send a single CRLF in response to a double CRLF", e);
}
continue;
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/gov/nist/javax/sip/parser/SIPMessageListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public interface SIPMessageListener extends ParseExceptionListener {
*/
public void processMessage(SIPMessage msg) throws Exception;

public void sendSingleCLRF() throws Exception;
public void sendSingleCRLF() throws Exception;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though this seems like a clear typo, i'm concerned this would break backwords compatibility... 🤔

Maybe a default implementation here for the new one invoking the old one would help us get the best of both worlds...

@jaimecasero thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this interface is not part of JAIN-SIP spec, and is not a boundary with the App. Is an internal API to allow IO layer to contact Parsing layer. If we change this interface, we will need to cover all the neccesary changes tin IO and parsing layer (XPipelineParsers,ConnOrientedMsgChannel,TCPMsgChannel,TLSMsgChannel...).

not sure is worth the change, but we could do it if the PR is comprehensive and tests results are ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggests to handle in different PRs, as the original concern about double CRLF on TCP closing seems valid. We could potentially merge this asap, and then think about the typo...

}
/*
* $Log: not supported by cvs2svn $
Expand Down
6 changes: 3 additions & 3 deletions src/gov/nist/javax/sip/stack/ConnectionOrientedMessageChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,7 +602,7 @@ public void run() {
int nbytes = myClientInputStream.read(msg, 0, bufferSize);
// no more bytes to read...
if (nbytes == -1) {
hispipe.write("\r\n\r\n".getBytes("UTF-8"));
hispipe.write("\r\n".getBytes("UTF-8")); // send \r\n to allow the pipe to wake up
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about wrapping this in a new method, e.g.
sendCrlfToWakeUpPipe(hispipe)

then we don't need comments!! ; ) \ o /

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure we can savely assume this branch is becuase of socket closed, certainly the inputstream has reached end. I can see the original author probably choosed doubleCRLF becuase is an small message,yet still valid under the heartbeat Spec. sending a single CRLF may cuase issues for some remote peers, who knows?

If the original concern of the PR is that " sometimes this will lead to the error message", why dont we try to review the log level of that message?. Jain-SIP stack is known to be quite verbose,and not using log level conventions properly. We have tried to alleviate this several times by reducing the log level, but there might be still some messages to tweak.

Copy link
Contributor Author

@fre42 fre42 Feb 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsaslis: Yes, I think the only reason for sending a double CRLF was to wakeup the pipe. Therefore the new method would cover it.
@jaimecasero: when sending the double CRLF the to wake up the pipe the pipe will handle this double CRLF and tries to send a single CRLF. This does not make sense in the case of shutting down. Therefore I was wondering why to send just this double CRLF where there are other possibilities to wake up the queue.
Unfortunately it's not only one error message that comes up in the case where mySock is still there and not closed in sendSingleCRLF() and the sendMessage() is called (which causes the errors). This is a race condition. The error messages looks ugly when it happens:

2018-02-05 19:22:37,842 WARN nist.javax.sip.stack.IOHandler [] IOException occured retryCount 0
2018-02-05 19:22:37,842 ERROR nist.javax.sip.stack.IOHandler [] Problem sending: sendBytes TCP inAddr 12.23.165.59 port = 57550 remoteHost 12.23.165.59 remotePort 57550 peerPacketPort 57550 isClient false
2018-02-05 19:22:37,842 ERROR nist.javax.sip.stack.IOHandler [] {}
2018-02-05 19:22:37,842 ERROR nist.javax.sip.stack.IOHandler [] Could not connect to /12.23.165.59:57550
2018-02-05 19:22:37,842 WARN nist.javax.sip.stack.TCPMessageChannel [] Failed to connect /12.23.165.59:57550 but trying the advertised port=57550 if it's different than the port we just failed on
2018-02-05 19:22:37,842 WARN nist.javax.sip.stack.TCPMessageChannel [] Couldn't connect to peerAddress = /12.23.165.59 peerPort = 57550 key = tcp:12.23.165.59:57550 retrying on peerPortAdvertisedInHeaders 57550

try {
if (sipStack.maxConnections != -1) {
synchronized (messageProcessor) {
Expand Down Expand Up @@ -720,9 +720,9 @@ public InetAddress getPeerPacketSourceAddress() {

/*
* (non-Javadoc)
* @see gov.nist.javax.sip.parser.SIPMessageListener#sendSingleCLRF()
* @see gov.nist.javax.sip.parser.SIPMessageListener#sendSingleCRLF()
*/
public void sendSingleCLRF() throws Exception {
public void sendSingleCRLF() throws Exception {
lastKeepAliveReceivedTime = System.currentTimeMillis();

if(mySock != null && !mySock.isClosed()) {
Expand Down