[reSIProcate] Matching INFO requests and responses

Thu Dec 9 08:44:53 CST 2010

Hi,

What about the following: rather than changing the DUM callbacks we could
expose a reader to get the last Contents that was sent as a NIT? A bit like
InviteSession::getRemoteSdp().

That way, if someone wants to access it, it can simply be done in the on*
callbacks. It would be cleaner in the sense that it won't change the
interfaces at all.

I will take care of ensuring that the memory is properly managed (helped by
valgrind). I was thinking of cloning the contents of the message then
assigning it to an auto_ptr as a member of InviteSession.

I also want to code a test suite to ensure that it works for INFO, MESSAGE.
We also need to ensure that it doesn't screw up the REFERs since currently
nitComplete() is also called from Dialog.cxx.

I was thinking of creating a branch to do this: b-nit-contents-20101209.
Scott, let me know if you think that would be a bad idea ;)

I'll let you know of my progress.

Francis

On Thu, Dec 9, 2010 at 1:43 AM, Aron Rosenberg <arosenberg at logitech.com>wrote:

> I coded up a working patch based on Scott's feedback. The only
> difference is that neither set of callbacks are pure-virtual now. I
> figure this was better moving forward so that new code didn't need to
> reference the old handler. It also seems that dealing with the
> responses to INFO and MESSAGE aren't needed by most normal UA's. I
> tested this patch with MESSAGE requests on the wire and it works.
>
> My only worry with this patch is that the memory handling semantics
> are correct. mNITQueue holds SharedPtr<SipMessage> and it seems ok for
> that to stay valid after dum.send() has been called. On the callback
> we pass a const Contents* rather than an auto_ptr since we can't get
> an auto_ptr from SipMessage for Contents without a clone call. I
> figure the developer who needs this longer than the life of the
> callback can clone themselves.
>
> Francis: Can you test the INFO side to make sure you are getting the
> right data back.
>
> -Aron
>
>
> On Wed, Dec 8, 2010 at 1:56 PM, Scott Godin <sgodin at sipspectrum.com>
> wrote:
> > I think we could add the new callback and leave the old one place.  The
> new
> > handler would not be virtual, and the default handler for the new
> callback
> > would just throw away the contents and call the old callback.  This will
> > allow current applications to use the new dum version without any code
> > changes.  For applications that want to see the Contents, they would need
> to
> > implement both callbacks, but the old callback can be left blank as it
> will
> > never get called.
> > Scott
> >
> > On Wed, Dec 8, 2010 at 4:36 PM, Aron Rosenberg <arosenberg at logitech.com>
> > wrote:
> >>
> >> The major use-case we need to fix is this:
> >>
> >> InviteSession based MESSAGE requests where the far end returns 415
> >> (mime-type invalid) for some types (text/plain works, but text/html
> >> doesn't, application/im-iscomposing+xml doesn't). Without the
> >> Contents* of the request that failed, we can't tell which type failed
> >> to determine if we need to notify the user sending the message.
> >>
> >> Scott: You have an opinion on if changing the callback type is an ok
> >> way to proceed?
> >>
> >> Francis: I have attached a prototype patch for this, it has only been
> >> compile tested if you want to try it and flesh it out some more.
> >>
> >> -Aron
> >>
> >> On Wed, Dec 8, 2010 at 12:39 PM, Francis Joanis
> >> <francis.joanis at gmail.com> wrote:
> >> > Hi,
> >> > I like what you are proposing since any user-specific information
> would
> >> > go
> >> > in the user-derived Contents object: it acts a bit like an
> AppDialog/Set
> >> > in
> >> > that sense.
> >> > We could even implement this by only caching the Contents of the last
> >> > sent
> >> > NIT request, since they're all sent serially (so no need to do the
> extra
> >> > management on mNITQueue). This would mainly require adding a new
> member
> >> > variable (probably an auto_ptr<Contents>) to InviteSession and
> changing
> >> > the
> >> > various on* callbacks in InviteSessionHandler.
> >> > In the case of the REFER (which is also using mNITQueue), it would
> most
> >> > likely have a NULL Contents, so it could get ignored on the callbacks
> >> > (i.e.
> >> > no need to modify the onRefer* callbacks).
> >> > I started playing around with the code but I'd like to know if such a
> >> > change
> >> > would be a good one or not from the perspective of the more
> experienced
> >> > developers. Changing callback APIs seems like a somewhat major change
> >> > (i.e.
> >> > it will break current apps) and I wouldn't want to do it unless there
> is
> >> > really a good use case.
> >> > Regards,
> >> > Francis
> >> >
> >> >
> >> > On Wed, Dec 8, 2010 at 1:32 PM, Aron Rosenberg <
> arosenberg at logitech.com>
> >> > wrote:
> >> >>
> >> >> For a slightly different issue (matching 415's to outbound MESSAGE
> >> >> requests) I was looking at modifying InviteSession to remove the
> >> >> SipMessage* from the mNITQueue upon end of transaction rather than
> >> >> beginning of transaction. We could then pass up the Contents* like is
> >> >> done with ClientPagerMessage.
> >> >>
> >> >> onMessageSuccess(resip::InviteSessionHandle ish, const
> >> >> resip::SipMessage&status) would become
> >> >> onMessageSuccess(resip::InviteSessionHandle ish, const
> >> >> resip::SipMessage&status, std::auto_ptr<Contents> contents)
> >> >> onMessageFailure(resip::InviteSessionHandle ish, const
> >> >> resip::SipMessage &status) would become
> >> >> onMessageFailure(resip::InviteSessionHandle ish, const
> >> >> resip::SipMessage &status, std::auto_ptr<Contents> contents)
> >> >>
> >> >> This would also work in the INFO cases with the same changes to add
> >> >> std::auto_ptr<Contents> contents to the callbacks.
> >> >>
> >> >> I also attempted to try the other way, dig into the
> >> >> TransactionController / TU and create a "SipMessage*
> >> >> findLastRequestForResponse(SipMessage*)" that used the tid, but this
> >> >> would require adding a number of public functions to SipStack,
> >> >> TransactionMap and some other classes.
> >> >>
> >> >> -Aron
> >> >>
> >> >> Aron Rosenberg
> >> >> Logitech Inc. (SightSpeed Group)
> >> >>
> >> >>
> >> >>
> >> >> On Wed, Dec 8, 2010 at 10:08 AM, Francis Joanis
> >> >> <francis.joanis at gmail.com> wrote:
> >> >> > So I guess it leaves me with the original workaround of leveraging
> >> >> > the
> >> >> > fact
> >> >> > that all NITs are sequential. Also I think I could use an outgoing
> >> >> > DumFeature to inspect outgoing messages, but that would be called
> for
> >> >> > all
> >> >> > outgoing messages, which will impact performance.
> >> >> > Thanks,
> >> >> > Francis
> >> >> >
> >> >> > On Wed, Dec 8, 2010 at 11:28 AM, Scott Godin <
> sgodin at sipspectrum.com>
> >> >> > wrote:
> >> >> >>
> >> >> >> The branch parameter is "reset" in DialogUsageManager send (ln
> 824)
> >> >> >> -
> >> >> >> so
> >> >> >> it won't help to store the branch parameter in the message created
> >> >> >> (same
> >> >> >> applies for onReadyToSend).
> >> >> >> Scott
> >> >> >>
> >> >> >> On Wed, Dec 8, 2010 at 11:08 AM, Francis Joanis
> >> >> >> <francis.joanis at gmail.com>
> >> >> >> wrote:
> >> >> >>>
> >> >> >>> Hi Scott,
> >> >> >>> Thanks for your reply.
> >> >> >>> What about if we changed InviteSession::info/refer/message to
> >> >> >>> return a
> >> >> >>> _copy_ of the constructed SipMessage rather than returning void?
> >> >> >>> I ran a quick test and it looks like the transaction id is set
> when
> >> >> >>> the
> >> >> >>> message is constructed. That way, I can immediately cache the SIP
> >> >> >>> transaction id to pair it with my "key". This removes the need
> for
> >> >> >>> my
> >> >> >>> extra
> >> >> >>> "key list" and does not require message inspection in
> >> >> >>> onReadyToSend.
> >> >> >>> (I've also attached a patch to do this.)
> >> >> >>> Thanks,
> >> >> >>> Francis
> >> >> >>>
> >> >> >>> On Wed, Dec 8, 2010 at 9:12 AM, Scott Godin
> >> >> >>> <sgodin at sipspectrum.com>
> >> >> >>> wrote:
> >> >> >>>>
> >> >> >>>> ...inline...
> >> >> >>>>
> >> >> >>>> On Tue, Dec 7, 2010 at 8:40 PM, Francis Joanis
> >> >> >>>> <francis.joanis at gmail.com> wrote:
> >> >> >>>>>
> >> >> >>>>> Hi guys,
> >> >> >>>>> I have a question about sending multiple NIT messages logically
> >> >> >>>>> "at
> >> >> >>>>> the
> >> >> >>>>> same time" (with DUM). I do know that the current code doesn't
> >> >> >>>>> allow
> >> >> >>>>> for
> >> >> >>>>> real parallel NIT requests (i.e. the NIT queue in
> InviteSession)
> >> >> >>>>> -
> >> >> >>>>> but
> >> >> >>>>> that's not my issue.
> >> >> >>>>> The issue I have is regarding how to match incoming INFO
> >> >> >>>>> responses
> >> >> >>>>> to
> >> >> >>>>> their request. Imagine I need to send INFO messages regarding
> >> >> >>>>> application-specific transactions (like DB transactions or
> >> >> >>>>> whatever
> >> >> >>>>> application specific thing like a button click). I'll call
> those
> >> >> >>>>> non-SIP
> >> >> >>>>> transaction ids "keys".
> >> >> >>>>> If I can't have the keys passed into the INFO body and then
> have
> >> >> >>>>> them
> >> >> >>>>> resupplied back into the INFO response, there is no easy way of
> >> >> >>>>> matching
> >> >> >>>>> which response is for which request (especially if my
> application
> >> >> >>>>> would
> >> >> >>>>> allow multiple requests to be sent (queued in NIT queue) at the
> >> >> >>>>> same
> >> >> >>>>> time).
> >> >> >>>>> The "workaround" would be to leverage the fact that NITs in an
> >> >> >>>>> INVITE
> >> >> >>>>> dialog are all serialized: I could keep a separate list of
> >> >> >>>>> "keys".
> >> >> >>>>> If I were
> >> >> >>>>> to add my key to the list right before sending the INFO
> message,
> >> >> >>>>> I
> >> >> >>>>> would
> >> >> >>>>> then know that the next INFO response will relate to that key
> >> >> >>>>> (oldest
> >> >> >>>>> element of the key list). But this can get messy since I would
> >> >> >>>>> need
> >> >> >>>>> to make
> >> >> >>>>> sure that the "key list" is properly managed.
> >> >> >>>>
> >> >> >>>> [Scott]  This solution works without having to modify DUM, and
> its
> >> >> >>>> likely very similar to how it would be implemented in DUM if we
> >> >> >>>> added
> >> >> >>>> some
> >> >> >>>> kind of key/id to the info() call and response callback.
> >> >> >>>>  Alternatively, you
> >> >> >>>> could track the transaction id's as the INFO messages flow out
> of
> >> >> >>>> the
> >> >> >>>> invite
> >> >> >>>> session by watching the onReadyToSend callback, but I don't
> think
> >> >> >>>> that
> >> >> >>>> offers any advantage over assuming the requests are serialized.
> >> >> >>>>
> >> >> >>>>>
> >> >> >>>>> If I look at the similar scenario but with INVITE messages, one
> >> >> >>>>> can
> >> >> >>>>> easily leverage the AppDialogSet/... to do this: set the "key"
> on
> >> >> >>>>> the
> >> >> >>>>> AppDialogSet and then it will be easily accessible once INVITE
> >> >> >>>>> responses
> >> >> >>>>> arrive (extracted from the INVITE session).
> >> >> >>>>> I was thinking of extracting the id of the SIP transaction
> >> >> >>>>> created
> >> >> >>>>> for
> >> >> >>>>> the INFO to use it for a lookup between the SIP transaction id
> >> >> >>>>> and
> >> >> >>>>> my "key".
> >> >> >>>>> That way, when handling onInfo* callbacks I would be able to
> >> >> >>>>> access
> >> >> >>>>> the SIP
> >> >> >>>>> transaction id from the response message and map it with my
> >> >> >>>>> "key".
> >> >> >>>>> However, this might not be feasible if the SIP transaction id
> is
> >> >> >>>>> unknown to the DUM at this time (prior to sending it).
> >> >> >>>>> What do you make out of this?
> >> >> >>>>> I suspect this will get worst if reSIProcate ever allows
> multiple
> >> >> >>>>> NITs
> >> >> >>>>> in parallel at the same time (in an INVITE dialog), since then
> >> >> >>>>> there
> >> >> >>>>> would
> >> >> >>>>> really be no easy way of matching them up to application
> specific
> >> >> >>>>> data (or
> >> >> >>>>> am I asking for an AppTransaction class ;)?)
> >> >> >>>>
> >> >> >>>> [Scott] Not sure we would ever allow this - I remember this
> being
> >> >> >>>> discouraged by the IETF.
> >> >> >>>>
> >> >> >>>>>
> >> >> >>>>> Thanks a lot,
> >> >> >>>>> Francis
> >> >> >>>>> _______________________________________________
> >> >> >>>>> resiprocate-devel mailing list
> >> >> >>>>> resiprocate-devel at resiprocate.org
> >> >> >>>>>
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
> >> >> >>>>
> >> >> >>>
> >> >> >>
> >> >> >
> >> >> >
> >> >> > _______________________________________________
> >> >> > resiprocate-devel mailing list
> >> >> > resiprocate-devel at resiprocate.org
> >> >> > https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
> >> >> >
> >> >
> >> >
> >>
> >> _______________________________________________
> >> resiprocate-devel mailing list
> >> resiprocate-devel at resiprocate.org
> >> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
> >
> >
>
> _______________________________________________
> resiprocate-devel mailing list
> resiprocate-devel at resiprocate.org
> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://list.resiprocate.org/pipermail/resiprocate-devel/attachments/20101209/d22730bd/attachment.htm>