[OpenSER-Devel] discussion: issues with local_route

[Daniel-Constantin Mierla]

On 06/23/08 23:58, Dan Pascu wrote:
> On Monday 23 June 2008, Daniel-Constantin Mierla wrote:
>   
>> Hello,
>>
>> On 06/20/08 13:49, Bogdan-Andrei Iancu wrote:
>>     
>>> Hi Daniel,
>>>
>>> To summarize :
>>>
>>> 1) script flag - indeed there is an issue there and fortunately the
>>> fix trivial - please open a bug report just not to forget about it
>>>
>>> 2) branch and branch_flags related function - these are not intended
>>> to be used in local  route as it makes no sense. As, said, the core
>>> does not allow any possibility to disable the core functions, so do
>>> not use it. If you use it (against the description), you may loose
>>> some info. But again, it is not a bug or missing, it is just about
>>> using inappropriate functions in a route.
>>>
>>> 3) time PVs - as time as the PVs are bind to the message (by design),
>>> it is normal to be refreshed when the processed message changes.
>>>
>>> I tried to addressed all your remarks that have a technical base. If
>>> I missed some, please list them along arguments.
>>>       
>> I started to believe you don't understand exactly what happens. You now
>> blame functions and functionalities that are there for ages, instead of
>> allowing time for a proper analysis, design and integration of the
>> local_route. You claimed that local route is a small patch with no
>> impact on architecture or other code, perhaps it is no longer valid.
>>     
>
> What exactly is broken? If I run openser with my 1.3 script will it work 
> exactly the same or not?
Could you please review the messages in the beginning of this email 
thread? I listed couple of them there, giving examples and details.

Again in summary...

 local_route changed the internal architecture of openser -- while so 
far there was only one SIP message context while running the route 
blocks (incoming sip message), now we have nested message contexts. 
Entire code of openser has to be reviewed to ensure proper 
functionality. Functions and functionalities so far were designed and 
implemented within the old architecture.

When local_route was introduced I asked time for other developers to 
review their code and properly integrate with local_route -- I saw that 
was understood as a run over the modules and enabling local_route for 
many functions, not letting the developers to do it.

As initial arguments were that there is no architectural change, no big 
impact over other code, I waited for the reaction to my technical 
reasons, which took about 1.5 weeks. I didn't want to rush anything, but 
allow time to sort out properly the issues.

I have never said to remove the code, but to enclose it in defines and 
disable it for 1.4. This is, in my opinion, the safest solution to have 
a coherent and robust 1.4 release. If decision is to go for fixing the 
code with local_route enabled, then will be kind of chaos, each 
developer will fix as it considers his part of code, either disabling it 
for local_route, because code was not designed for nested environments 
and does not want to load unnecessary bug reports, the problem being the 
design of local_route, or making other workarounds. Probably the 
local_route will be pretty much unusable.

I have no time during this testing phase to go over entire code I wrote 
in the last 8 years and do a proper analysis, so I will disable it for 
local_route at root level.



>  And if not is it a matter of a simple bug fix, 
> or a complete redesign of the local_route?
> So far I've seen only 1 issue that may affect the script flags and it's 
> easy to fix.
>
>   
>> If you do not log branches, branch flags in acc or xlog, does not mean
>> nobody else is doing. It is poor excuse to go with such motivation -
>> the application must be coherent and consistent.
>>     
>
> Yet, we have the same issues with reply_route and error_route. There are 
> operations that make no sense there either. If you try them, they are 
> either silently ignored or you get an error. What makes the local_route 
> inconsistencies more relevant, while the other inconsistencies do not 
> bother you?
>   
Just read the initial messages and you will understand. There are side 
effects, big ones and critical.


> You do remember that when first introduced, AVPs could not be accessed in 
> the reply_route.
Let me expose the right history. When avps where introduced, the only 
functions managing them in config were from avpops where the functions 
flags denied usage in reply_route.

When the avps could be used directly in the config file, you could use 
them anywhere and they were and still are safe. I believe you mix the 
avp attached to transaction (those attached to request) and avps 
attached to reply. If you play with the parameter of the TM module, then 
you get in onreply_route the transaction avps, otherwise you get the 
avps attached to the reply.

>  It required another few releases until the ability to 
> access them in the reply route was made possible. Following this logic, 
> we should have never added the AVP support in the first place since it 
> lacked this ability and even though some people did not use it, some 
> others may have wanted to, but were unable.
>
>   
>> So, please, avoid argumentations with "inappropriate usage" or "not
>> designed to be used there (but allowed)", it is just your opinion,
>> besides that quality software and projects don't go out with such stuff
>> in major releases.
>>     
>
> But this is no singular case, and to speak in its defense I have to say 
> that it's very new, so one cannot expect it to cover any use case, even 
> those that were not imagined yet.
>
> If it has to go down this path, then why don't we pick up on some 
> functionality that is already in openser for 4+ generations and it has 
> issues. TCP/TLS support is in there since 0.9.0 and today we still can't 
> use it for end user devices. The consensus when this issue is raised, is 
> that it is only supported for server connections, but I can also say that 
> why should the end user device use case be labeled "inappropriate usage" 
> when we target to release quality software?
>
> So what do you say, if I connect 20000 SIP phone via TCP/TLS to the proxy 
> will it be able to work properly as it does with UDP? If not, what should 
> we do, should we remove TCP/TLS support because it is incomplete for this 
> use case?
>   
I do not understand the point and relation with the discussion. Sounds 
like, just as stupid example, if 10 billions processes do better than 
10billions threads, shall the  multi-thread support in kernel be removed?

>   
>> To go further and complete the 1.4 release, the solution I see is to
>> enclose local_route in defines in disable it by default. Who wants to
>> use it, can recompile and reinstall from sources.
>>     
>
> As I already said, I will support you in this direction if we can find any 
> problems that break the existing proxy functionality and cannot be fixed 
> easily. However if there are no show stoppers, only incomplete stuff, I 
> have to disagree. Every new functionality was incomplete in the 
> beginning, and required multiple releases until it reached maturity and 
> sometimes even required design changes between those releases to allow it 
> to be implemented in a more complete form. Nothing new with local_route.
>   

Again, it is nothing about completeness, it is about breaking lot of 
other things, changing the internal architecture. It is not about 
supporting one or another, any developer has the right to rise and 
expose the problems, not agree when something is broken and take care of 
its code to be protected.

Cheers,
Daniel

-- 
http://www.asipto.com