Skip to content

Destroy rd_kafka_t handle on consumer.close() (#30) #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
edenhill merged 1 commit into from
Aug 23, 2016
Merged

Destroy rd_kafka_t handle on consumer.close() (#30) #33

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Destroy rd_kafka_t handle on consumer.close() (#30)
  • Loading branch information
@edenhill
edenhill committed Aug 19, 2016
commit 3b26f017ceba2eb192a5ae344e5e76d329c7fa55
8 changes: 7 additions & 1 deletion confluent_kafka/src/Consumer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,12 +391,18 @@ static PyObject *Consumer_poll (Handle *self, PyObject *args,

static PyObject *Consumer_close (Handle *self, PyObject *ignore) {
CallState cs;
int raise = 0;

CallState_begin(self, &cs);

rd_kafka_consumer_close(self->rk);

if (!CallState_end(self, &cs))
raise = !CallState_end(self, &cs);
Copy link
Contributor

@ewencp ewencp Aug 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@edenhill edenhill Aug 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@ewencp ewencp Aug 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edenhill Hmm, I guess it doesn't matter much then. Seems fine as is and this LGTM.


rd_kafka_destroy(self->rk);
self->rk = NULL;

if (raise)
return NULL;

Py_RETURN_NONE;
Expand Down