Re: [reSIProcate] Matching INFO requests and responses
The only "main" reason to expand the callback API was to try and match
ClientPagerMessageHandler...but I can rework the API to still keep the
last request in the NIT queue but expose a new method on InviteSession
called getLastNITRequest() . Should that return a
auto_ptr<SipMessage>, const SipMessage* or something else?
-Aron
On Thu, Dec 9, 2010 at 6:53 AM, Scott Godin <sgodin@xxxxxxxxxxxxxxx> wrote:
> 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@xxxxxxxxx>
> 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@xxxxxxxxxxxx>
>> 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@xxxxxxxxxxxxxxx>
>>> 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@xxxxxxxxxxxx>
>>> > 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@xxxxxxxxx> 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@xxxxxxxxxxxx>
>>> >> > 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>>> >> >> > 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@xxxxxxxxx>
>>> >> >> >> 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@xxxxxxxxxxxxxxx>
>>> >> >> >>> wrote:
>>> >> >> >>>>
>>> >> >> >>>> ...inline...
>>> >> >> >>>>
>>> >> >> >>>> On Tue, Dec 7, 2010 at 8:40 PM, Francis Joanis
>>> >> >> >>>> <francis.joanis@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
>>> >> >> >>>>>
>>> >> >> >>>>> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>>> >> >> >>>>
>>> >> >> >>>
>>> >> >> >>
>>> >> >> >
>>> >> >> >
>>> >> >> > _______________________________________________
>>> >> >> > resiprocate-devel mailing list
>>> >> >> > resiprocate-devel@xxxxxxxxxxxxxxx
>>> >> >> > https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>>> >> >> >
>>> >> >
>>> >> >
>>> >>
>>> >> _______________________________________________
>>> >> resiprocate-devel mailing list
>>> >> resiprocate-devel@xxxxxxxxxxxxxxx
>>> >> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>>> >
>>> >
>>>
>>> _______________________________________________
>>> resiprocate-devel mailing list
>>> resiprocate-devel@xxxxxxxxxxxxxxx
>>> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>>
>>
>> _______________________________________________
>> resiprocate-devel mailing list
>> resiprocate-devel@xxxxxxxxxxxxxxx
>> https://list.resiprocate.org/mailman/listinfo/resiprocate-devel
>
>