Avoid sending double CRLF keep alive on TCP socket shutdown

[fre42]

The current implementation does send a duplicate CRLF in the case of a TCP/TLS connection shutdown (socket closed). This leads to the situation that the PipelinedMsgParser handles it as "RFC 5626 CRLF keepalive mechanism" and tries to send a single CRLF. This sending will of course fail because the socket is closed.
Due to a race condition when closing the socket, sometimes this will lead to the error message "A problem occured while trying to send a single CRLF in response to a double CRLF" which is no error.
By the way I've fixed a small typo: "CRLF" was sometimes "CLRF" in log messages an in the code.

[gsaslis]

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

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

[jaimecasero]

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.

[fre42]

@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

[gsaslis]

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?

[jaimecasero]

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

[jaimecasero]

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...

[gsaslis]

@fre42 Thanks for submitting the PR ❤️ - really appreciate you taking the time to contribute this!!

Could you please take a look at our Open Source Playbook and then proceed with signing our CLA, so we can then move forward with properly reviewing this?

Thanks in advance!
Yorgos

[fre42]

I need to clarify with my company if I'm allowed to sign the CLA. Unfortunately this may take some time.

[gsaslis]

@fre42 sure thing.

Please take your time, so we can make sure we do this properly.

[fre42]

Where can I find this CLA? I've found the link https://www.restcomm.com/contributor-license-agreement/#Contribute. The CLA is mentioned there but not linked.

[gsaslis]

@fre42 sorry for the delay in getting back to you on this.

The CLA is available on that page "inside" the form... you have to select "Individual" or "Corporate" on the form at the bottom of the page for the CLA text to come up.

(I know it's not ideal, won't be long before we integrate CLA signing as part of the PR flow)

[fre42]

@gsaslis Meanwhile I've got feedback from my company's legal department. They have concerns signing the CLA as it is:
From a legal standpoint, the license is worded to grant

"… Telestax, its successors and assigns the non-exclusive, transferable, irrevocable, perpetual, royalty-free right to use, modify, copy, sell, distribute”, “without limitation”

, any of our patents that may be included in a contribution, i.e. not tied to the contribution. OSS licenses usually contain wording to limit such license to the use of the contribution within the license terms, see below example from the EPL 1.0 license.
Eclipse Public License 1.0, section 2 b):

Subject to the terms of this Agreement, each Contributor hereby grants Recipient a non-exclusive, worldwide, royalty-free patent license under Licensed Patents to make, use, sell, offer to sell, import and otherwise transfer the Contribution of such Contributor, if any, in source code and object code form. This patent license shall apply to the combination of the Contribution and the Program if, at the time the Contribution is added by the Contributor, such addition of the Contribution causes such combination to be covered by the Licensed Patents. The patent license shall not apply to any other combinations which include the Contribution.

They don’t think that this was the intention of Telestax, however, it is what the agreement says.
We would therefore be o.k. to agree on limiting terms, e.g.:

This grant is made with respect to any copyright, patent or other intellectual property or moral rights (together: “IPR”) Contributor may have in or to the Contributions, as far as the exercise of the rights granted in the preceding sentence would otherwise infringe such IPR.

Would such an individual CLA be OK for you?

[gsaslis]

@fre42 thanks for bringing that to my attention - will check internally and let you know asap.

[gsaslis]

@fre42 sorry for the delay here - can you please drop me an email at yorgos.saslis youKnowWhatGoesHere telestax.com, so we can continue this discussion and not take this thread off-topic?