Destroy rd_kafka_t handle on consumer.close() (#30) #33
Conversation
edenhill
force-pushed
the
destroy_on_close
branch
from
ab402c2 to
3b26f01
Compare
|
|
||
| CallState_begin(self, &cs); | ||
|
|
||
| rd_kafka_consumer_close(self->rk); | ||
|
|
||
| if (!CallState_end(self, &cs)) | ||
| raise = !CallState_end(self, &cs); |
There was a problem hiding this comment.
My understanding of the behavior here is that we essentially want to do the equivalent of Consumer_dealloc, but as far as I can tell, the rd_kafka_destroy is only a fraction of what should actually be done? Why isn't the full Consumer_clear (or more completely, Consumer_dealloc) performed? In particular, since the full Consumer_clear is not performed, doesn't this mean we'll leave some callbacks un-decref'd?
There was a problem hiding this comment.
Yes, almost, dealloc() is eventually triggered by the GC and I left it for that to clean up any lingering objects (such as callbacks), but they will be cleaned up on the next GC spin.
We could call cleanup from close() as an optimization to speed up GC, apart from that there shouldnt be any functional differences.
What do you think?
There was a problem hiding this comment.
@edenhill Hmm, I guess it doesn't matter much then. Seems fine as is and this LGTM.
Solves issue #30