[sr-dev] reset dst_uri in t_load_contacts

[Jan Janak]

On Sat, Oct 17, 2009 at 9:41 AM, Juha Heinanen <jh at tutpro.com> wrote:
> Jan Janak writes:
>
>  > I am wondering if calling clear_branches is enough. Shouldn't the
>  > function also call reset_dst_uri to reset the value of the dst_uri
>  > (which is now also stored in the AVP), and perhaps also clear the send
>  > socket, the branch flags and the path vector for the Request-URI?
>
> jan,
>
> may be so, but i have thought that script writer would not have set
> dst_uri or send socket if script is calling next_contacts().  so i would
> consider it a bug in the script if those things are set when
> next_contacts() is called. on the other hand, it might not hurt to clear
> them.

I wasn't actually thinking that the script writer would set them
explicitly, but I though that it might be a good idea to have them
cleared when t_load_contact finishes. But after reading your reply I
realized that this is in fact not necessary, because a subsequent call
to t_next_contacts rewrites them anyway. We just have to make sure
that t_next_contacts properly clears those fields that have empty
values in the AVP.

This makes sense, because if t_load_contacts was successful then there
is at least one contact stored in the AVP and it is guaranteed that at
least the Request-URI will be updated when t_next_contacts is called
for the first time. But it is not guaranteed that any of the branches
will be updated, this can happen if we have just one contact with the
highest q value, therefore we have to clear them.

I also wonder if t_load_contacts is the right place to clear branches.
I think that it would be better to do that in the first call to
t_next_contacts. However, figuring out whether we call t_next_contacts
for the fist time can be difficult. We would probably need to store
this information in the contacts AVP somehow. At least for me
personally, having the branches cleared in t_load_contacts is kinda
unexpected side-effect. We should probably document it in the README
(I can take care of that, I want to update the README anyway).

>  > All these values are encoded into the AVP, but they are only cleared
>  > for additional branches (in clear_branches), but not for the
>  > Request-URI "branch".
>
> if r-uri and all branches (if any) have the same q, then load_contacts()
> does nothing and branches should not be cleared, because they are used
> "as is" without loading them from contacts_avp.

Yes, that's how it works, I checked that. t_load_contacts clears
branches only if it produces something that can actually by used by
t_next_contacts. And because t_load_contacts clears branches, the
script writer has to call t_next_contacts in this case, otherwise some
of the contacts could be lost.

Things could break if the script writer calls t_load_contacts and does
not call t_next_contacts. I suggest that we change this after the
release and make t_next_contacts to clear the branches if needed. Then
nothing bad would happen if the script writer only calls
t_load_contacts, it will just create the AVP but if you do not call
t_next_contacts then things would work as usual, the request will be
forked to all branches in parallel.

This way it would be also possible to fill the AVP with contacs from
other sources than t_load_contacts (i.e. the database). That is not
possible right now, because t_next_contacts does not clear branches
for itself--t_load_contacts has to be called first.

  -- Jan