[asterisk-dev] [Code Review] 4108: Weak Proxy Objects

Tue Mar 31 16:52:55 CDT 2015

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4108/#review14995
-----------------------------------------------------------

This approach is much simpler and easier to understand than previous attempts.  Since it is more general it could be used to avoid mutual ref issues such as between the stasis_topic and stasis_subscription as described in main/stasis.c.

All of the issues I've pointed out are minor.

/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/4108/#comment25613>

    You could do it this way:
    /*! \brief Macro which must be used at the beginning of each sorcery capable object */
    #define SORCERY_OBJECT(details)                    \
    struct {                                           \
    	struct ast_sorcery_object_details details; \
    }                                                  \

    This way you aren't using a macro that alters the declaration syntax.

    Need to document that principle intent of user defined fields on weak proxy object structs is to serve as immutable container keys for the real object.

/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/4108/#comment25622>

    Why not use the OBJ_NOLOCK flag?

/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/4108/#comment25623>

    Why not use the OBJ_NOLOCK flag?

/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/4108/#comment25624>

    Why not use the OBJ_NOLOCK flag?

/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/4108/#comment25626>

    ...as many times.

/trunk/include/asterisk/astobj2.h
<https://reviewboard.asterisk.org/r/4108/#comment25625>

    Why not use the OBJ_NOLOCK flag?

/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment25615>

    For alignment purposes, this should be grouped with the other pointer (destructor_fn).

    Also ao2_weak -> ao2_weakproxy

/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment25616>

    How about renaming to:
    IS_AO2_MAGIC_BAD()

    if (IS_AO2_MAGIC_BAD()) {
      ast_log(LOG_ERROR, "Bad magic number\n")
    }

/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment25620>

    Bumping weakproxy is not necessary here.  It is just done as a convienience to later coding.

    There is an expense to ast_atomic_fetchadd_int() because of syncing processors.

/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment25621>

    Isn't the shell game you are playing with the real object's ref count going to mess up the DEBUG_REF output?

    How about this code:
    Code assumes that weakproxy ref wasn't bumped when set above.
    if weakproxy
       if current_value == 1
          unlink real and weak
       unlock weakproxy
       if current_value == 1
          run weakproxy callbacks (locking between each callback or locking to run all)
          unref weakproxy
          unref real

/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment25617>

    I think you need to have a weakproxy destructor callback that performs a sanity check on the proxy object to ensure that it is not still linked with a real object.  

    Or even better, make the ao2_ref code do this check when it is about to destroy a weakproxy object.  This check can also destroy anything in the notify list for the case if the real object was not set but subscribers were.

/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment25618>

    Format nit, break lines at weaker binary operations and put the operation at the beginning of the indented new line:
    if (!weakproxy_internal
        || weakproxy_internal->priv_data.magic != AO2_WEAK) {
        return -1;
    }

    if (!obj_internal
        || obj_internal->priv_data.weakptr
        || obj_internal->priv_data.magic != AO2_MAGIC) {
        return -1;
    }

    This way you aren't as likely to miss the binary operator because it isn't hiding at the end of a possibly still long line.

    And don't use spaces for indentation.

/trunk/main/astobj2.c
<https://reviewboard.asterisk.org/r/4108/#comment25619>

    I don't see that this goto really helps here.

    Invert the test and pull the skipped code into the then block.

/trunk/tests/test_astobj2_weaken.c
<https://reviewboard.asterisk.org/r/4108/#comment25627>

    Digium didn't create this file.

/trunk/tests/test_astobj2_weaken.c
<https://reviewboard.asterisk.org/r/4108/#comment25633>

    Value not checked at end of test to ensure that it got destroyed and there were no leaks.

/trunk/tests/test_astobj2_weaken.c
<https://reviewboard.asterisk.org/r/4108/#comment25630>

    ++*i
    avoids the need for parentheses and post increment isn't really needed.

/trunk/tests/test_astobj2_weaken.c
<https://reviewboard.asterisk.org/r/4108/#comment25628>

    It is best if you do one declaration per line.  There have been numerous times where one of the variables changes type or is deleted.
    void *obj;
    void *obj2;
    void *strong1;

/trunk/tests/test_astobj2_weaken.c
<https://reviewboard.asterisk.org/r/4108/#comment25629>

    ...alloc weakref1

/trunk/tests/test_astobj2_weaken.c
<https://reviewboard.asterisk.org/r/4108/#comment25631>

    weakref2

/trunk/tests/test_astobj2_weaken.c
<https://reviewboard.asterisk.org/r/4108/#comment25632>

    Destrustor and notifies not called...

- rmudgett

On March 4, 2015, 3:43 p.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4108/
> -----------------------------------------------------------
> 
> (Updated March 4, 2015, 3:43 p.m.)
> 
> 
> Review request for Asterisk Developers, George Joseph and rmudgett.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This implements "weak" references.  The weakproxy object is a real ao2 with normal reference counting of its own.  When a weakproxy is pointed to a normal object they hold references to each other.  The normal object is automatically freed when a single reference remains (the weakproxy).  The weakproxy also supports subscriptions that will notify callbacks when the normal object is about to be destroyed.
> 
> 
> Diffs
> -----
> 
>   /trunk/tests/test_astobj2_weaken.c PRE-CREATION 
>   /trunk/main/astobj2.c 432445 
>   /trunk/include/asterisk/astobj2.h 432445 
> 
> Diff: https://reviewboard.asterisk.org/r/4108/diff/
> 
> 
> Testing
> -------
> 
> Ran the included test with REF_DEBUG enabled under valgrind.  No reference leaks or improper memory access.  Though this does not test for races, I don't know of an automated way to do that.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150331/d76244ff/attachment-0001.html>