Skip to content

fix(eventhubs): stop authorization refresh on close - #5119

Draft
Johnathan W (j7nw4r) wants to merge 5 commits into
Azure:mainfrom
j7nw4r:fix/eventhubs-producer-memory-leak
Draft

fix(eventhubs): stop authorization refresh on close#5119
Johnathan W (j7nw4r) wants to merge 5 commits into
Azure:mainfrom
j7nw4r:fix/eventhubs-producer-memory-leak

Conversation

@j7nw4r

Copy link
Copy Markdown
Member

Summary

Event Hubs ProducerClient close cycles retain authorization refresh task references and can cause sustained heap growth.

Motivation

The owned authorization refresh task keeps connection and authorizer references alive during connection teardown. This prevents producer lifecycle memory from being released promptly and matches the reported growth of about 4.8 KB per cycle.

Changes

Stops the owned authorization refresh task before RecoverableConnection teardown and yields once so task-held references are released before close returns.

Adds a deterministic regression test for the owned authorization refresh task and a live allocator test covering 100 lifecycles after five warmup cycles.

Test plan

  • cargo fmt -p azure_messaging_eventhubs -- --check passed.
  • cargo clippy -p azure_messaging_eventhubs passed.
  • cargo build -p azure_messaging_eventhubs --all-features passed.
  • cargo test -p azure_messaging_eventhubs --all-features passed with 145 unit tests and 45 doc tests; live tests skipped because Event Hubs environment variables were absent.
  • The focused deterministic regression passed and git diff --check origin/main...HEAD passed.
  • The live allocator integration test was not run because the required Event Hubs environment variables were absent.

Closes #4595

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
3 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Track live allocation bytes across sequential producer lifecycle tests.
Use warmups, block medians, and a noise-adjusted regression bound.
Gate a refresh credential and verify close releases the authorizer task.
Exercise the connection-owned authorizer with a gated refresh credential.
Expose only test-gated helpers for deterministic refresh timing and setup.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Stops Event Hubs authorization refresh tasks during connection closure to prevent retained producer lifecycle memory.

Changes:

  • Cancels refresh tasks before connection teardown.
  • Adds deterministic refresh-task cleanup coverage.
  • Adds a live heap-growth regression test.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/common/authorizer.rs Adds refresh-task cancellation and test configuration.
src/common/recoverable/connection.rs Stops refresh during close and adds regression coverage.
tests/eventhubs_producer_memory.rs Adds lifecycle heap-trend testing.
Suppressed comments (2)

sdk/eventhubs/azure_messaging_eventhubs/tests/eventhubs_producer_memory.rs:70

  • A missing Event Hub name also reports this regression test as passed without running any lifecycle. Preserve the fallback name if needed, but propagate failure when neither variable is configured.
    let eventhub = match env::var("EVENT_HUB_NAME") {
        Ok(eventhub) if !eventhub.is_empty() => eventhub,
        _ => match env::var("EVENTHUB_NAME") {
            Ok(eventhub) if !eventhub.is_empty() => eventhub,
            _ => return Ok(()),

sdk/eventhubs/azure_messaging_eventhubs/tests/eventhubs_producer_memory.rs:76

  • Credential construction failure silently turns the live regression into a passing no-op. Since live mode was explicitly requested, propagate this error so an unusable test environment is visible.
    let credential = match DeveloperToolsCredential::new(None) {
        Ok(credential) => credential,
        Err(_) => return Ok(()),
    };

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +119 to +123
pub(crate) async fn stop_refresh_task(&self) {
if let Some(task) = self.authorization_refresher.get() {
task.abort();
}
get_async_runtime().yield_now().await;
Comment on lines +62 to +65
let host = match env::var("EVENTHUBS_HOST") {
Ok(host) if !host.is_empty() => host,
_ => return Ok(()),
};
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.

Eventhub ProducerClient does not free memory on close()

2 participants