Safely release resources in Handle_clear #434
Conversation
|
It looks like @coldeasy hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here. Once you've signed reply with Appreciation of efforts, clabot |
|
Good catch @coldeasy, this actually came up very recently as t causes issues for python, .net and likely our golang client as well. In order to handle this more elegantly the following PR has been opened for librdkafka. Once merged the plan is to adapt python to use the new immediate closure flags confluentinc/librdkafka#1932 |
|
Thanks for the quick reply @rnpridgeon. Would you consider merging this to create a patch release in the meantime? |
| @@ -1324,24 +1324,37 @@ static void log_cb (const rd_kafka_t *rk, int level, | |||
| * Clear Python object references in Handle | |||
| */ | |||
| void Handle_clear (Handle *h) { | |||
| if (h->error_cb) | |||
| Py_DECREF(h->error_cb); | |||
| if (h->error_cb) | |||
There was a problem hiding this comment.
Use 8 spaces for indent.
Put open-brace on same line as if:
if (h->error_cb) {
There was a problem hiding this comment.
Updated, please take another look.
|
|
||
| if (h->initiated) { |
There was a problem hiding this comment.
no whitespace/indent changes please
During garbage collection tp_clear and tp_dealloc can both be called which resulted in a double release of references. This change safely decrements the reference count and invalidates the pointers.
|
[clabot:check] |
|
@confluentinc It looks like @coldeasy just signed our Contributor License Agreement. 👍 Always at your service, clabot |
|
Thank you for this! |
During tests we create and destroy instances of
ConsumerandProduceron every test case. We noticed segfaults and memory corruption (theloggerpointer was referring to other random objects).This seems to be due to the case where both
tp_clearandtp_deallocare called on the handle, resulting in a double release oferror_cb,stats_cbandlogger. I've tested this locally and have not encountered the segfaults/memory corruption since.