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

AMQP changes for C++ Claim Based Security - also fixed session close issue that affected reliability #1833

Merged

Conversation

LarryOsterman
Copy link
Member

@LarryOsterman LarryOsterman commented Oct 2, 2024

This pull request introduces several important changes to the azure_core_amqp and azure_messaging_eventhubs SDKs. The most significant updates involve adding lifetime parameters to various structs and implementations to ensure proper borrowing and memory management. Additionally, there are some minor improvements and refactorings to enhance code clarity and error handling.

Lifetime Parameter Additions:

Refactorings and Improvements:

  • sdk/core/azure_core_amqp/src/fe2o3/connection.rs: Simplified error handling and removed unnecessary unwrap calls by using if let instead of ok_or_else. [1] [2]
  • sdk/eventhubs/azure_messaging_eventhubs/src/consumer/mod.rs and sdk/eventhubs/azure_messaging_eventhubs/src/producer/mod.rs: Updated AmqpClaimsBasedSecurity::new calls to pass a reference to AmqpSession instead of taking ownership. [1] [2]

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

The generated summary covers the what, but that's obvious in a fairly small PR. I'm curious as to the why. Is it because the AmqpSession was being copied? Was the Arc<> causing delays?

@LarryOsterman
Copy link
Member Author

The generated summary covers the what, but that's obvious in a fairly small PR. I'm curious as to the why. Is it because the AmqpSession was being copied? Was the Arc<> causing delays?

The problem was that when the types underlying the session were copied, somehow the underlying fe2o3::session::SessionHandle got dropped. That shouldn't have been possible, but the traces were unambiguous: The underlying AMQP layer felt it was necessary to drop the session handle. Possibly that was because I didn't clone the session properly, I honestly don't know.

However, while I was trying to figure out why the sessionhandle was dropped, I realized that the session lifetime was always going to be longer than the CBS lifetime, so instead of taking a reference to the underlying Arc, I could just take a reference to the session. And once I switched from taking a reference to the Arc<Mutex<SessionHandle>> to taking a reference to the Session, the problem went away.

@LarryOsterman LarryOsterman merged commit c7b6c14 into Azure:feature/track2 Oct 3, 2024
28 checks passed
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.

2 participants