RestComm / Restcomm-Connect

The Open Source Cloud Communications Platform
http://www.restcomm.com/
GNU Affero General Public License v3.0
244 stars 215 forks source link

P2P text message fails to return a 200 OK to the sender #691

Closed atsakiridis closed 7 years ago

atsakiridis commented 8 years ago

Scenario:

The problem is that no 200 OK is returned to Android Olympus and the request times out. RestComm issues the following exception at that point:

09:24:35,552 ERROR org.mobicents.servlet.restcomm.sms.SmsService Parse error reading content null with content type text/plain: java.lang.IllegalArgumentException: Parse error reading content null with content type text/plain at org.mobicents.servlet.sip.message.SipServletMessageImpl.setContent(SipServletMessageImpl.java:1391) at org.mobicents.servlet.restcomm.telephony.util.B2BUAHelper.forwardResponse(B2BUAHelper.java:467) [restcomm.telephony.api-7.4.0.1209.jar:] at org.mobicents.servlet.restcomm.sms.SmsService.response(SmsService.java:350) [restcomm.sms-7.4.0.1209.jar:] at org.mobicents.servlet.restcomm.sms.SmsService.onReceive(SmsService.java:340) [restcomm.sms-7.4.0.1209.jar:] at akka.actor.UntypedActor$$anonfun$receive$1.applyOrElse(UntypedActor.scala:159) [akka-actor_2.10-2.1.2.jar:] at akka.actor.ActorCell.receiveMessage(ActorCell.scala:425) [akka-actor_2.10-2.1.2.jar:] at akka.actor.ActorCell.invoke(ActorCell.scala:386) [akka-actor_2.10-2.1.2.jar:] at akka.dispatch.Mailbox.processMailbox(Mailbox.scala:230) [akka-actor_2.10-2.1.2.jar:] at akka.dispatch.Mailbox.run(Mailbox.scala:212) [akka-actor_2.10-2.1.2.jar:] at akka.dispatch.ForkJoinExecutorConfigurator$MailboxExecutionTask.exec(AbstractDispatcher.scala:506) [akka-actor_2.10-2.1.2.jar:] at scala.concurrent.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:262) [scala-library-2.10.1.jar:] at scala.concurrent.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:975) [scala-library-2.10.1.jar:] at scala.concurrent.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1478) [scala-library-2.10.1.jar:] at scala.concurrent.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:104) [scala-library-2.10.1.jar:] Caused by: java.lang.NullPointerException: null content at gov.nist.javax.sip.message.SIPMessage.setContent(SIPMessage.java:1312) [jain-sip-ri-7.0.4.231.jar:7.0.4.231] at org.mobicents.servlet.sip.message.SipServletMessageImpl.setContent(SipServletMessageImpl.java:1386) ... 13 more

@gvagenas, @croufay: for full logs please check:

https://gist.github.com/atsakiridis/eec500705055bd99d1bf

I'm using RestComm AMI 7.4.0.1209

P.S. I get a similar issue when using Web Olympus as the sender P.S.2. Also reproducible from linphone

atsakiridis commented 8 years ago

@croufay tried your fix and first time it worked fine, but second it didn't. I made some more attempts and it seems it sporadically fails :( (about 1/4 times).

Here's the new log at the line of the exception:

https://gist.github.com/atsakiridis/93bf5e21f43d25f79902#file-restcomm-server-log-L164

atsakiridis commented 8 years ago

Still getting this issue. Here are latest logs:

https://gist.github.com/atsakiridis/4af472024c20803fc77012955ab3da0c

Please follow Call-Id: 849495f8abcc80b450571d615983cb19

200 OK is returned to restcomm from the recepient, but restcomm cannot forward back to sender

gvagenas commented 8 years ago

@atsakiridis from the logs I can see that after client "despina" sends 200 OK, the container invalidates the sip session and thus the NPE.

11:05:58,474 INFO  [org.mobicents.servlet.sip.core.session.SipSessionImpl] (MSS-Executor-Thread-6) Invalidating the sip session (88298658_f507ab6e_57a5b08a_36eb6786;65ff16da1748051cf7d69b08334ad382@172.30.2.84;36eb6786;RestComm)

What I will do is the following:

  1. After the message from client "antonis" is proxied to client "despina", Restcomm will return 202 Accepted to client "antonis"
  2. B2BUAHelper will check the sip session of the response from client "despina" and only if valid will try to proxy it to client "antonis", thus we will avoid the NPE.

@deruelle can you take a look to the logs also and check why container invalidated the sip session of the client? Restcomm doesn't invalidates the sip session at any point.

deruelle commented 8 years ago

@gvagenas I don't think we should do 1. because if the Message can't be delivered, the user will think it was delivered successfully. @gvagenas can you protect the getState with isValid too first ? What are we doing at RC level with the session that just got a 200 OK ( it makes sense to invalidate given this is an out of dialog request so it should be terminated fast albeit it may be too fast here) @atsakiridis can you provide similar traces in DEBUG mode for org.mobicents and giv.nist ?

atsakiridis commented 8 years ago

@gvagenas @deruelle: So the actual issue is SipServlets invalidating the session very soon, right? Cause from the clients' perspective I can't see anything wrong; the recipient seems to send back the 200 OK answer right away.

Let me try to collect those traces locally

gvagenas commented 8 years ago

@deruelle since this is an out of dialog request isn't ok to response with 202 which means Restcomm accepted the message?

Here is the link to the code when RC receives the 200 OK - https://github.com/RestComm/Restcomm-Connect/blob/d1a69baa904b2053a253c043b66facb54a173059/restcomm/restcomm.telephony.api/src/main/java/org/mobicents/servlet/restcomm/telephony/util/B2BUAHelper.java#L480-L480 - and this is the place that will check if the sip session is still valid:

         if (response.getSession() != null && response.getSession().isValid() &&
                 (!response.getSession().getState().equals(SipSession.State.TERMINATED))) {

For now I will not merge anything to the master branch.

deruelle commented 8 years ago

@gvagenas how would you communicate to the client that RC couldn't deliver the message if the delivery fails. I think we will need to review that, the solution makes sense long term where on 202, the Client could show a sent indicator and maybe once delivered send back another message to the initial client to say the message was delivered somehow to display a delivered indicator.

@gvagenas I think the next line will throw NPE as well https://github.com/RestComm/Restcomm-Connect/blob/d1a69baa904b2053a253c043b66facb54a173059/restcomm/restcomm.telephony.api/src/main/java/org/mobicents/servlet/restcomm/telephony/util/B2BUAHelper.java#L484 maybe we should get it at the beginning of the method.

gvagenas commented 8 years ago

@deruelle agree on your point for 202.

About the next line, th method checks for sip session validity, check here: https://github.com/RestComm/Restcomm-Connect/blob/d1a69baa904b2053a253c043b66facb54a173059/restcomm/restcomm.telephony.api/src/main/java/org/mobicents/servlet/restcomm/telephony/util/B2BUAHelper.java#L420-L420

deruelle commented 8 years ago

yes but if not valid it will return a null session which will make getAttribute of https://github.com/RestComm/Restcomm-Connect/blob/d1a69baa904b2053a253c043b66facb54a173059/restcomm/restcomm.telephony.api/src/main/java/org/mobicents/servlet/restcomm/telephony/util/B2BUAHelper.java#L484 throw an NPE.

gvagenas commented 8 years ago

@deruelle I see what you mean, will check this also

gvagenas commented 8 years ago

@deruelle @atsakiridis I was thinking that in case the response session gets invalidated, we could send an error response to the first client, what do you think? what error would be more appropriate to this case?

deruelle commented 8 years ago

@gvagenas instead of storing in sipsession why don't we store in sipapplicationsession ? This one won't get invalidated as long as there is one session alive

gvagenas commented 8 years ago

@deruelle B2BUAHelper was designed for call sessions so sip session makes sense. Maybe we can have a B2BUAHelper for the SIP MESSAGE requests only and use the sip application session there.

deruelle commented 8 years ago

@gvagenas actually you can ask the container not to watch the session with sipsession.setInvalidateWhenReady(false) so it doesn't invalidate it, see 6.2.4.1.2 Invalidate When Ready Mechanism in the spec. but don't forget to invalidate both sessions once the 200 OK to original session has been sent otherwise we will have leaks