Rafael H. Schloming commented on PROTON-829:

What really motivates the current scheme is not the pooling, but the need to 
avoid circular references. The object graph simply has circular references and 
there is no reasonable way to avoid this in the current design, yet all these 
objects need to be wrappable into python, which means you need to be able to 
present an incref/decref style memory management interface. As far as I can 
tell, that pretty much demands that you have a hybrid refcount/incremental gc 
scheme of some sort. I fully agree that pn_incref(...)/pn_decref(...) is an 
incredibly obscure way to trigger an incremental garbage collect though and the 
code needs to be better factored to make the design more transparent.

> Possible reference counting bug in pn_clear_tpwork
> --------------------------------------------------
>                 Key: PROTON-829
>                 URL: https://issues.apache.org/jira/browse/PROTON-829
>             Project: Qpid Proton
>          Issue Type: Bug
>          Components: proton-c
>    Affects Versions: 0.8
>            Reporter: Alan Conway
>            Assignee: Alan Conway
>             Fix For: 0.9
> See QPID-6415 which describes a core dump in the qpid tests that appears when 
> using the current 0.9 proton master. The qpid tests pass OK with proton 0.8.
> The valgrind output in QPID-6415 shows that a connection is deleted while it 
> is being finalized by  a call from pn_connection_unbound to pn_clear_tpwork.
> I do not yet understand the details, but removing the following strange code 
> fixes the problem and passes the proton test suite without valgrind errors:
> {noformat}
> --- a/proton-c/src/engine/engine.c
> +++ b/proton-c/src/engine/engine.c
> @@ -690,10 +690,10 @@ void pn_clear_tpwork(pn_delivery_t *delivery)
>    {
>      LL_REMOVE(connection, tpwork, delivery);
>      delivery->tpwork = false;
> -    if (pn_refcount(delivery) > 0) {
> -      pn_incref(delivery);
> -      pn_decref(delivery);
> -    }
>    }
>  }
> {noformat}
> The code is strange because
> a) you should never examine a refcount except for debugging purposes
> b) under normal refcounting semantics incref+decref is a no-op.
> Is removing this code OK?

This message was sent by Atlassian JIRA

Reply via email to