On Sat, Oct 17, 2009 at 9:41 AM, Juha Heinanen jh@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