-
Notifications
You must be signed in to change notification settings - Fork 346
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
fix: prevent rpc client panic on rpc response if ProducerReady
is nil
#973
Conversation
/pulsarbot run-failure-checks |
seems like CI failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you show how to reproduce it? In which case could this field be nil
? We should not perform nil checks everywhere. It could hide some potential bugs.
@BewareMyPower Yes, you're right. Pulsar Broker: v2.7.4 I just created a simple producer to send messages on an existed topic, and it worked with v0.9.0. However, after upgrading the pulsar-client-go to the master branch, I encountered a panic error. It appears that the panic is related to PR #958 I don't know why the if res.error == nil && *res.RPCResult.Response.Type == pb.BaseCommand_PRODUCER_SUCCESS {
if res.RPCResult.Response.ProducerSuccess.ProducerReady == nil {
timeoutCh = nil
break
}
} My modifications involve using ProducerSuccess.GetProducerReady() instead of ProducerSuccess.ProducerReady, which would always return true and prevent a panic when producer_ready is nil |
The reason is that the I just see the default value of |
@BewareMyPower Thanks for solving my doubts |
Motivation
prevent rpc client panic on rpc response if
ProducerReady
is nilModifications
use
ProducerSuccess.GetProducerReady
instead ofProducerSuccess.ProducerReady
panic log
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation