Skip to content

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

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

Open
fre42 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

Changes from 1 commit
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
2 changes: 1 addition & 1 deletion 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

@gsaslis gsaslis Feb 6, 2018

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

@jaimecasero jaimecasero Feb 6, 2018

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