Skip to content

Fix panic when response failed by timeout #396

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

Merged

Conversation

yurahaid
Copy link
Contributor

Fix Panic on Response Timeout in handleWriteWithResponse

image

When it happened:

  1. You declare a publisher, but for some reason, the response from the RabbitMQ is delayed
  2. handleWriteWithResponse handle timeout and remove response from coordinator
  3. Delayed response arrived from Rabbitmq into handleGenericResponse, but GetResponseById can't find the response because it was deleted in step 2, and we got a panic

hiimjako
hiimjako previously approved these changes May 15, 2025
Copy link
Collaborator

@hiimjako hiimjako left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Gsantomaggio, shouldn't this logic apply to all message types? Currently, sometimes errors are only logged, while other times they are logged and the function returns without sending.

@yurahaid yurahaid force-pushed the fix-panic-when-repsonse-not-found branch from cdb5777 to de4e6a0 Compare May 15, 2025 11:17
@yurahaid
Copy link
Contributor Author

yurahaid commented May 15, 2025

I agree with @hiimjako.
I think if we get an error from c.coordinator.GetResponseById(readProtocol.CorrelationId), we should always avoid using Response because it will be nil`

If you're ok with it, I will add a new commit to the current PR to fix it.

@Gsantomaggio
Copy link
Member

If you're ok with it, I will add a new commit to the current PR to fix it.

Yes. Thank you.

@Gsantomaggio Gsantomaggio merged commit c039132 into rabbitmq:main May 16, 2025
2 checks passed
@Gsantomaggio
Copy link
Member

Thank you @yurahaid and @hiimjako for the review

@yurahaid yurahaid deleted the fix-panic-when-repsonse-not-found branch May 19, 2025 08:22
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

Successfully merging this pull request may close these issues.

3 participants