[L2Ork-dev] Delayed pd_unbind

[Ivica Ico Bukvic]

Albert,

Thanks for reporting this. I will need to look into this before I can say
more. I do remember a reason for this delayed system being something along
the lines of: if you change send/receive symbol in the same cycle as sending
a message this can cause things to segfault because the previous symbol
ends-up being null pointer that is referenced by the same traversal. There
is a thread somewhere on pd-list talking about this and me also posting an
example (or at least having one to work with based on the description of the
problem). Given that I've never run into problems of having memory leaks or
segfaults, I am also wondering what is faust doing that is different from
the usual and if so, whether the design flaw is on that end of things. This
is because every time something is passed the global variable is tripped and
immediately cleared by the end of that particular cycle of messages is
processed. This has, at least in my experience proved robust and I was
unable to notice any memory leaks/issues. If you have any vanilla example
where this is apparent, please do send it. Otherwise, I would want to
investigate further as to what exactly is faust doing that necessitates
access to this low-level system.

The call should guarantee traversal over every list given because all
bindlist_* calls account for it within the same loop (below is an excerpt
from bindlist_float, but the same should hold true for all other types as
well):

static void bindlist_float(t_bindlist *x, t_float f)
{
    t_bindelem *e;
        change_bindlist_via_graph = 1;
    for (e = x->b_list; e; e = e->e_next)
        if (e->e_who != NULL) pd_float(e->e_who, f);
        if (change_bindlist_via_graph > 1)
                bindlist_cleanup(x);
        change_bindlist_via_graph = 0;
}

So, as far as I can tell, things are taking place sequentially and are
guaranteed to take place every time. That is, unless faust is somehow using
pd_unbind in an unconventional way. 

That said, you did identify one obvious bug (namely line 266) and thank you
very much for doing so. I will add that aspect to the next release.

Best wishes,

Ico

> -----Original Message-----
> From: l2ork-dev-bounces at disis.music.vt.edu [mailto:l2ork-dev-
> bounces at disis.music.vt.edu] On Behalf Of Albert Graef
> Sent: Tuesday, November 12, 2013 4:26 PM
> To: l2ork-dev at disis.music.vt.edu
> Subject: [L2Ork-dev] Delayed pd_unbind
> 
> Hi Ico,
> 
> I've run into some issues with code added to the pd_unbind function in
> pd/src/m_pd.c, which apparently was introduced some time in March 2013
> (see e.g. https://github.com/pd-l2ork/pd/commit/78ff67b). This seems
> to be a quick hack to delay the freeing of entries in bind lists while
> one of the bindlist_xyz operations is in progress.
> 
> I'm not sure why this is actually needed, but in any case the modified
> code has some serious problems. In particular, it causes segfaults in
> my pd-faust external which works fine with both vanilla pd and
> pd-extended. I could track this down to the following lines near the
> end of pd-l2ork's pd_unbind (see m_pd.c:276):
> 
>         for (e = b->b_list; e; e = e->e_next)
>         {
>             if (e->e_who != NULL)
>                 count_valid++;
> 
>         }
>         if (count_valid == 1) {
>             s->s_thing = b->b_list->e_who;
>             if (!change_bindlist_via_graph) {
>                 freebytes(b->b_list, sizeof(t_bindelem));
>                 pd_free(&b->b_pd);
>             }
>             //fprintf(stderr,"success B2\n");
>         }
> 
> This code is supposed to get rid of an explicit bind list if it
> shrinks to size 1, storing the receiver directly in the symbol
> instead. The line 's->s_thing = b->b_list->e_who;' is the main culprit
> here. It works fine in the vanilla Pd code, but not in your modified
> code, because there it isn't guaranteed any more that the single
> remaining receiver is at the head of the list. So in some cases
> s->s_thing gets set to the wrong receiver object, which may cause
> segfaults later on when Pd tries to deliver some message to an object
> which might not exist any more (this is actually why I got those
> segfaults in my external).
> 
> Also, there seems to be a typo at line 266. I think it should be:
> 
>                 e2->e_delayed_free = 1; // not 0
> 
> But even with this change, the code still leaks memory like a sieve.
> That's because there is no guarantee that if pd_unbind is called
> during a call to one of the bindlist_xyz functions, that it will be
> called on the same object that is being traversed. But the calls to
> bindlist_cleanup in the bindlist_xyz functions will only free bind
> list entries in the traversed object, not any other objects whose bind
> lists happen to be updated during the call, resulting in memory leaks.
> 
> Moreover, bindlist_cleanup seems to assume that there's at most one
> entry to free per binding list, which in general doesn't hold either
> AFAICT. Also, the use of a single global flag
> change_bindlist_via_graph is troublesome, since it doesn't take into
> account recursive calls to the bindlist_xyz functions. I actually
> noticed this in my external, where bindlist_cleanup wasn't being
> called at all, since recursive bindlist_xyz calls would reset the
> change_bindlist_via_graph flag.
> 
> These are the most obvious flaws that I found while reviewing the
> code, there might be more. I think that fixing all these issues and
> coming up with a really robust implementation of the delayed freeing
> will require considerable effort. Are you sure that this is needed?
> What problem are you trying to solve there? Neither vanilla pd nor
> pd-extended seems to need this.
> 
> Since I haven't been able to fix the code, I just reverted pd-l2ork's
> changes in pd_bind/pd_unbind for now, using the pd_bind/pd_unbind code
> from vanilla Pd instead. This works fine for me. I can submit a
> corresponding pull request if you want, just let me know.
> 
> Cheers,
> Albert
> 
> --
> Dr. Albert Gr"af
> Dept. of Music-Informatics, University of Mainz, Germany
> Email:  aggraef at gmail.com
> WWW:    https://plus.google.com/+AlbertGraef
> _______________________________________________
> L2Ork-dev mailing list
> L2Ork-dev at disis.music.vt.edu
> http://disis.music.vt.edu/listinfo/l2ork-dev