confluentinc / confluent-kafka-python

Confluent's Kafka Python Client
http://docs.confluent.io/current/clients/confluent-kafka-python
Other
84 stars 892 forks source link

Fix segfault with describe_topics and flaky connection #1692

Closed lpsinger closed 4 months ago

lpsinger commented 9 months ago

If you call describe_topics on a flaky connection, sometimes the admin client reply has the host set to a null pointer. When this occurs, instead of segfaulting, report the host as None.

cla-assistant[bot] commented 9 months ago

CLA assistant check
All committers have signed the CLA.

cla-assistant[bot] commented 9 months ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

pranavrth commented 6 months ago

Thanks for the PR.

Even though the changes seems correct, I think we should not change the base implementation which is being used in alot of APIs. Some of the values of other APIs must not be NULL. I think its better to handle the scenario in this way - https://github.com/confluentinc/confluent-kafka-python/blob/master/src/confluent_kafka/src/Admin.c#L3198-L3201.

I am also checking if it should be possible to get the host information as NULL or should we handle it as an error instead. But in the meantime you can rebase the branch and do the change mentioned above if you have time. Otherwise, you can wait for my revert.

lpsinger commented 6 months ago

But in the meantime you can rebase the branch and do the change mentioned above if you have time.

Done.

pranavrth commented 5 months ago

/sem-approve

lpsinger commented 5 months ago

@emasab, what else do you want me to do with this?

emasab commented 5 months ago

@lpsinger after reviewing all the cases only in c_Node_to_py it's needed to check for NULL values because it takes the nodes from the Metadata request and some of then can be absent, so without host, when the broker is down and not listed there.

Could you leave only that change?

lpsinger commented 5 months ago

Could you leave only that change?

Done. Would you like me to squash the changes?

pranavrth commented 4 months ago

/sem-approve

pranavrth commented 4 months ago

/sem-approve

pranavrth commented 4 months ago

/sem-approve

lpsinger commented 4 months ago

You're very welcome!