[jira] [Commented] (PROTON-829) Possible reference counting bug in pn_clear_tpwork

["Rafael H. Schloming (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/PROTON-829?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14338377#comment-14338377 ] 

Rafael H. Schloming commented on PROTON-829:
--------------------------------------------

I added that code to get the python tests to pass without valgrind errors, so I'm surprised that removing it doesn't cause issues.

I'll try to explain a little bit about what's going on. (I've commented this elsewhere in the code, but I missed this spot. I think andrew pointed out that this might be clearer with a helper function instead of/in addition to a comment.)

There are definitely non-debug cases where it is useful to examine the refcount. In particular, a finalizer gets fired when the refcount hits zero, but the finalizer can sometimes run code that creates a new reference to the object that is being finalized. There are a few places where this pattern is very useful:

  - The finalizer can run some logic that decides whether or not the object should be pooled and if so sticking it in the pool adds a reference.
  - The finalizer might run some code that posts an event, thereby creating a reference from the collector to the object.
  - The connection/session/link/delivery/transport objects all have pointers to each other, and to just naively refcount all those pointers would result in circular references and memory leaks. In order to avoid circular references, we don't refcount internal pointers, e.g. the parent->child pointers, instead the finalizers of all the children have logic that runs and figures out if it is safe for the child to go away (basically does a tiny amount of incremental garbage collection). If the finalizer's "garbage collection" finds an internal reference, the parent of that object takes over the last external reference. In the latter case an external pointer to a child may be created again, and to account for this, the incref of all the child objects is overriden to remove the reference that the parent took over.

In all these cases, the logic that may/may not create a new reference is run at the beginning of the finalizer. The finalizer then needs to check if a new reference has actually been created in order to figure out whether or not to actually free the resources used by the object or to return. This pattern could perhaps be formalized by distinguishing between finalizers (the part that runs application logic which can create new references) and destructors (the part that actually frees anything malloced in the initializer) in the object code.

This should also explain why incref/decref is not necessarily a noop, it is basically retriggering the "gc" portion of the code in the finalizer that decides whether or not an object should really go away or not, and in the rare case that you toggle some state that might influence that gc logic, you need to get it to run again. Clearing the tp work queue is one such case. The reason it is guarded by a check on the refcount is that we also call that function from inside the delivery finalizer, and in that case if the refcount is zero we've already decided that the object should go away.

Again, this gc part of the pattern could probably be formalized a bit, e.g. possibly having some sort of mark/sweep style thing that is in fact triggered by external references reaching zero, but probably not a good idea to attempt anytime soon since what complicates this picture is not so much the fact that it isn't formalized, but rather that the engine uses ad-hoc data structures that predate the development of the collections that are part of the object library and therefore has to manually account and search for references in a type specific way rather than using the automatic ref counting and/or search routines available in the collections. This manual/type specific accounting is what makes the "gc" logic a bit brittle, and could possibly be the source of the bug you are seeing. At some point relatively soon (probably after 0.9 but before the next release) I expect to have to update a significant chunk of those data structures to properly use the collections, but until then if you manage to pare this down to a minimal reproducer in terms of proton API calls, that would definitely suggest the bug is in proton and I can probably help debug more from there. As it is, it looks like it could possibly just be a plain old double free style bug in messaging, and removing the above code just masks it by introducing a memory leak.

> 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
(v6.3.4#6332)