-
Notifications
You must be signed in to change notification settings - Fork 95
Add null check for consumer in MessageListenerProxy #219
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
Add null check for consumer in MessageListenerProxy #219
Conversation
| delete data; | ||
|
|
||
| jsCallback.Call({msg, consumer->Value()}); | ||
| if (consumer) { |
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.
Do we need to print some warning messages when the consumer is a null value?
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.
No. The initial value is null. And I found the NodeJS wrapper never logs any message. Even if you added a log here, there is still helpless to find in which scenario consumer is null here.
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.
I'm not sure if users lose to handle some messages when the consumer is null.
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.
I think the controversial point is whether should we skip the null consumer.
- Yes: might lose messages (not sure)
- No: fast fail
Adding logs doesn't solve anything. There is no logger in NodeJS client, printing logs to console is meaningless and helpless as I said.
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.
We need more info to reproduce the segmentation fault. But for now, a safe solution is to avoid accessing a null consumer.
|
@BewareMyPower Also, what versions of client libraries(pulsar-client-node and pulsar-client-cpp) were used when this segmentation fault occurred? |
|
Yeah. And I'll confirm the version from the user |
There is a segmentation fault reported by a user.
The
consumerfield is assigned fromMessageListenerCallbackand the initial value isnullptr. This field could only be modified inConsumer::SetListenerCallback. I didn't look deeper into where it's invoked, but I think it's better to add the null check here for safety.