[reSIProcate] Matching INFO requests and responses

[Scott Godin]

That works for me too - in fact if you are going with this approach, then
you may as well store the entire last NIT request and make it available
instead of just the contents.  A branch sounds like a good idea.  : )

Thanks,
Scott

On Thu, Dec 9, 2010 at 9:44 AM, Francis Joanis <francis.joanis at gmail.com>wrote:

> 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
>>
>
>
> _______________________________________________
> 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/c3b49fe5/attachment.htm>