[L2Ork-dev] Delayed pd_unbind

[Albert Graef]

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