< Previous by Date Date Index Next by Date >
< Previous in Thread Thread Index Next in Thread >

Re: [reSIProcate] Matching INFO requests and responses


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