[sr-dev] git:master: dialog: Don't destroy dialogs in timer while they' re still being referenced

[Daniel-Constantin Mierla]

On 04/11/14 14:19, Alex Hermann wrote:
> On Tuesday 04 November 2014, Daniel-Constantin Mierla wrote:
>> On 03/11/14 17:05, Alex Hermann wrote:
>>> On Monday 03 November 2014, Daniel-Constantin Mierla wrote:
>>> True. But even then refcounting shouldn't be bypassed. For each (cleanup)
>>> situation the expected refcount should be compared to the actual refcount
>>> before deleting.
>> Not sure what would be expected reference counter value in each of the
>> states.
> I think it is necessary to know it to be able to securely cleanup dialogs.
>
>
>> In theory, the dialog cannot last for ever in initial state, so cleaning
>> it up make sense to stay safe of memory leaks at runtime -- we can print
>> log messages to alert, but keeping it for ever brings more troubles
>> (e.g., besides memory leaks, the call limit can be affected).
> I agree those must be cleaned, even though it is not technically a memory 
> leak, as the dialog pointer is still used in the dialog hash.
>
>
>> As I said, for operations that are long term, I consider better to keep
>> dlg index and label (two integers) than increasing the reference count
>> and storing the pointer (on 64b it is the same size). Then lookup again
>> the dialog when needed.
> Who is talking about "long term" operations? The duration is not of any 
> relevance here.
>
> Almost none of the dialog related operations apart from getting the dlg 
> pointer are protected by locks but by the refcounter. Suppose process 1 
> obtains a dlg pointer (from the h_entry, and h_id), and upped the refcounter 
> as a result. It can get scheduled away at any random time, even directly after 
> obtaining the dlg pointer. If the timer process get scheduled at that moment 
> and frees the memory of the dlg (disregarding the fact that is is still in 
> use), how is process 1 to know that?
>
>
>> Finding the reason of what determined you to do
>> this is important, not to push kind of workaround that we don't fully
>> understand its purpose and benefits.
>>
>> IMO, it is not right to keep a dialog in proxy as long as caller/callee
>> don't have it anymore.
> I agree, just not with the method you used to get rid of the dialog.
>
>  
>> You can eventually make the behavior of the patch configurable, so you
>> can enable it via modparam, in the current form I don't find it ok.
> I'm not going to make a known segfault path configurable, so i'll revert it if 
> you insist. But may i kindly suggest you revert your code too which introduced 
> this code in the first place as, IMHO, it is fundamentally wrong by deleting 
> an object without checking if it is in use.

If you discovered a flaw in existing code, then should be fixed
properly. It is what I am trying to do as well by discussing your patch.
This is the benefit of open source, reviewing patches for mutual
benefits, and I do hope other people check my commits as well.

I didn't say you didn't get an issue with it and the existing old code
was perfect, I am just trying to understand the issue and be sure it is
the right fix, with no side effects. I will be more than happy to solve
what you found as problem in the old patch, together we what I found not
clear with the last patch.

You said you don't have a backtrace for it and also you didn't provide
any further details about the situation, but now you refer to it as a
'known segfault'. If you can provide more info about the crash, it can
help to really solve the problem.

Just reverting and going back in another buggy state it is not good. I
expressed my opinions that with the patch, it seems we can get back to
the state with dialogs stuck in memory for ever. The config was more
like a temporary solution if you had any pressure on having the patch,
to give everyone an easy option to decide how to use it.

Otherwise, yes, I do want to get it properly fixed, with no options.

Cheers,
Daniel

-- 
Daniel-Constantin Mierla
http://twitter.com/#!/miconda - http://www.linkedin.com/in/miconda
Kamailio Advanced Training, Nov 24-27, Berlin - http://www.asipto.com