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

Fixed #5536, fixed a test deadlock discovered while testing the fix for 5536, added some diagnostics for uAMQP to help track the problem. #5651

Merged
merged 6 commits into from
May 22, 2024

Conversation

LarryOsterman
Copy link
Member

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

  • C++ Guidelines
  • Doxygen docs
  • Unit tests
  • No unwanted commits/changes
  • Descriptive title/description
    • PR is single purpose
    • Related issue listed
  • Comments in source
  • No typos
  • Update changelog
  • Not work-in-progress
  • External references or docs updated
  • Self review of PR done
  • Any breaking changes?

Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

Thank you! I'm glad to see this fixed. It must not have been easy to repro this.

Add changelog?

BTW, for automatic issue linking and integration, I think you need to put Fixes #5536 in the PR description, not to the title.

@ahsonkhan
Copy link
Contributor

This PR is doing a few things. For my edification, either in the PR description, or as a reply, can you please summarize the root cause of the underlying issue, and highlight how this PR is fixing it? It would be helpful in reviewing the most relevant parts.

@LarryOsterman
Copy link
Member Author

This PR is doing a few things. For my edification, either in the PR description, or as a reply, can you please summarize the root cause of the underlying issue, and highlight how this PR is fixing it? It would be helpful in reviewing the most relevant parts.

IMHO the PR is doing one thing: Making the link test reliable. And most of the root causes are laid out in the text of the PR.

But here's a summary of the fixes:

  • Fixed a use-after-free error caused by unprotected access to a uAMQP API, specifically the link_create and link_create_from_endpoint APIs.
  • Fixed a deadlock caused by calling AddPollable while holding the connection lock.
  • Fixed a case in uAMQP where functions were called on a link before setting the value of the name field
  • Fixed an uninitialized member field in uAMQP, which could lead to freeing uninitialized memory.
  • Fixed a bug in uAMQP where the session_set_link_endpoint_callback method was not called from link_create_from_endpoint.

All these fixes were found either when running the test lots of times, or by diagnostics added trying to track down the bugs.

@LarryOsterman LarryOsterman merged commit 4fcf09d into Azure:main May 22, 2024
41 checks passed
@LarryOsterman LarryOsterman deleted the larryo/fixed_5536 branch September 6, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intermittent test failure for AMQP LinkAttachDetachMultipleOneSession - Message receiver link detached
3 participants