Skip to content
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.

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

Panic on Kafka IP change #1028

Open
canni opened this issue Mar 22, 2024 · 7 comments
Open

Panic on Kafka IP change #1028

canni opened this issue Mar 22, 2024 · 7 comments

Comments

@canni
Copy link

canni commented Mar 22, 2024

We're experiencing a panic due to send on a closed channel here: https://github.com/cloudevents/sdk-go/blob/release-v2.15.2/protocol/kafka_sarama/v2/receiver.go#L80

I've traced this to the fact that the incoming channel is closed in SDK managed goroutine here: https://github.com/cloudevents/sdk-go/blob/release-v2.15.2/protocol/kafka_sarama/v2/receiver.go#L182

But the ConsumeClaim is running in sarama's internal goroutine, and it's session context is unrelated. This creates situaction where closing the receiver prematurely closes the incoming channel used by ConsumeClaim

This panic is crashing the whole go process, as wrapping StartReceiver with recover() does not cover that goroutine.

@embano1
Copy link
Member

embano1 commented Mar 22, 2024

Hi @canni thx for filing: quick question -> are you running Confluent Kafka or another environment? We have a PR ready for Confluent Kafka support where we tried to make sure to avoid races when closing. Need to look closer into the sarama implementation or do you want to file a PR?

@embano1
Copy link
Member

embano1 commented Mar 22, 2024

@yanmxa FYI, that's why I was so picky during the PR review :)

@canni
Copy link
Author

canni commented Mar 22, 2024

Yes this is when we connect to cluster using confluent docker containers

@embano1
Copy link
Member

embano1 commented Mar 24, 2024

Would you be open to switch and try our upcoming kafka_confluent binding?

@canni
Copy link
Author

canni commented Mar 26, 2024

I don't know if we can find time to get around to testing that before Easter break, I'll report after the trial run

@embano1
Copy link
Member

embano1 commented Mar 26, 2024

No worries, #988 merge still pending (though close). Then we need to cut a new release so you can easily switch. Appreciate the support.

@embano1
Copy link
Member

embano1 commented Apr 14, 2024

@canni #988 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants