feat(eventhubs): add buffered producer client - #4873
feat(eventhubs): add buffered producer client#4873Johnathan W (j7nw4r) wants to merge 6 commits into
Conversation
f985c0c to
04f3e85
Compare
|
Azure Pipelines: Successfully started running 1 pipeline(s). 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds a buffered Event Hubs producer for bounded, asynchronous batching and delivery reporting.
Changes:
- Adds the buffered producer public API, routing options, delivery contexts, and
SendNotAccepted. - Implements partition workers, batching, backpressure, flushing, shutdown, and routing.
- Adds CBS authorization serialization, documentation, examples, and tests.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/eventhubs_buffered_producer.rs |
Adds live buffered-producer tests. |
src/producer/mod.rs |
Adds raw batch-envelope sending. |
src/producer/buffered/worker.rs |
Implements partition workers. |
src/producer/buffered/tests.rs |
Adds buffered-producer unit tests. |
src/producer/buffered/send_client.rs |
Abstracts production and mock sending. |
src/producer/buffered/partition_resolver.rs |
Implements partition assignment. |
src/producer/buffered/mod.rs |
Defines the client, builder, and lifecycle. |
src/producer/batch.rs |
Extracts owned batch serialization state. |
src/lib.rs |
Exports the new public API. |
src/error.rs |
Adds SendNotAccepted. |
src/common/recoverable/connection.rs |
Adds CBS serialization locking. |
src/common/recoverable/claims_based_security.rs |
Applies the CBS lock. |
README.md |
Documents buffered publishing. |
examples/eventhubs_buffered_produce_events.rs |
Adds a usage example. |
CHANGELOG.md |
Records the feature and CBS fix. |
## Summary
Two claims-based-security authorizations that overlap on one AMQP
connection make the link attach fail. The service permits only one
`$cbs` link for each connection, and it rejects the second attach with
`NotAllowed` ("A link to connection ... $cbs node has already been
opened"). This change puts the authorizations of one connection in
sequence.
## Motivation
`RecoverableConnection::ensure_amqp_cbs` attaches a new `$cbs` link for
each authorization, uses it, and then drops it. Nothing kept two of them
apart. One authorization at a time is safe, because the previous link is
gone before the next one attaches. Two authorizations that start at the
same time are not.
The client classifies `NotAllowed` as not retryable, so the failure
reaches the caller. The sender or receiver never attaches, and the
operation fails.
A client that attaches one link at a time never sees this, which is why
the fault stayed hidden. A client that sets up more than one link at
once does see it. The failure rate depends on the timing, so a retry of
the whole operation often succeeds and hides the cause.
## Changes
- Added a `cbs_lock` to `RecoverableConnection`, and a
`lock_claims_based_security` method that takes it.
- `RecoverableClaimsBasedSecurity::authorize_path` now holds that lock
for the full round trip, which covers the link attach, the put-token,
and the drop.
- Added the live test `send_to_every_partition_at_once`. It reads the
partitions first, so the connection is open, and it then sends to every
partition at the same time from one client. Only the sender attaches
overlap.
The lock covers only the authorization. The link attach that follows,
the session begin, and the sends and receives all stay concurrent, so
this does not undo the per-path concurrency work of #4563.
## Validation
`cargo fmt --check`, `cargo clippy --all-targets --all-features -- -D
warnings`, `cargo test --all-features` (122 unit tests and 45 doc tests,
no failures), and `RUSTDOCFLAGS='-Dwarnings' cargo doc` all pass.
**The new test ran against a real Event Hub with 5 partitions, and it
shows both states.** With this change, 3 runs of 3 pass, and all 5 sends
succeed in each run. With the lock removed and nothing else changed, 3
runs of 3 fail, at 3 of 5 sends each time. The trace of the failing runs
gives the cause directly: `NotAllowed, description: Some("A link to
connection '268' $cbs node has already been opened.")`.
The live tests `consumer_open_with_connection_string`,
`send_eventdata_with_connection_string`, and
`test_round_trip_connection_string` also pass, so the
single-authorization path does not regress.
The local test identity holds no `Send` role on the test namespace, so
the new test ran through a local change that authenticates with a
connection string. Its committed form uses `recording.credential()`,
which matches the other tests in that file.
#4873 hits the same fault through a different path. It adds a client
that starts one sender for each partition, and it carries this fix
today. That branch rebases and drops the duplicate commit after this
change merges.
e29ba5b to
e25ecc0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/worker.rs:423
- These permits are released only after the success/failure handler has finished. If this batch owns every permit for the partition, a handler that enqueues to the same partition deadlocks: its enqueue waits for capacity that this worker releases only after that handler returns. Release the counters and permits as soon as the send reaches its terminal outcome, before awaiting the delivery handler.
self.release(event_count);
// The events reached a terminal outcome, so the buffer has space again.
drop(permits);
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/mod.rs:632
- A concurrent second
closeorabortreturnsOkhere as soon as the first shutdown setsclosed, even though workers may still be sending and the connection is not yet released. This contradicts the shutdown methods' wait guarantees; a close/abort race also silently ignores the second caller's requested mode. Coordinate shutdown through shared completion state so every caller waits for the in-progress shutdown (and define close-versus-abort precedence).
async fn shutdown(&self, abandon: bool) -> Result<()> {
if self.closed.swap(true, Ordering::AcqRel) {
return Ok(());
sdk/eventhubs/azure_messaging_eventhubs/CHANGELOG.md:7
- This changelog entry is not concise: it combines the public API summary with detailed batching behavior, defaults, cross-SDK comparisons, and shutdown semantics. The changelog guideline requires each public API change to be summarized briefly on one line; keep the detailed behavior in the README and API docs.
- Added `BufferedProducerClient`, a producer that accepts single events and publishes them in the background. One worker owns each partition, so events keep their enqueued order and a slow partition does not stop the others. A successful enqueue means only that the local buffer accepted the event; the client reports each terminal outcome through the handlers that `with_on_send_succeeded` and `with_on_send_failed` register, and a handler for failed batches is required. The client sends a batch when the next event does not fit, when the batch reaches the configured event count, when the maximum wait time expires, on `flush`, and on `close`. The defaults are a 1 second maximum wait time and 1500 buffered events for each partition, matching the .NET, JavaScript, and Python clients. `flush` sets a barrier over the events accepted before the call, `close` sends the buffered events, and `abort` abandons them. New public types: `BufferedProducerClient`, `BufferedProducerClientBuilder`, `EnqueueEventOptions`, `SendBatchSucceededContext`, and `SendBatchFailedContext`. ([#4873](https://github.com/Azure/azure-sdk-for-rust/pull/4873))
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (3)
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/mod.rs:632
closedrecords that shutdown started, not that it completed, but every concurrent or later caller returnsOkimmediately. Thus a secondclose()/abort()can report success while the first call is still sending and invoking handlers; if the first future is cancelled or its producer close fails, no later call can finish or observe that failure. Use a shared shutdown completion/result (or an async lifecycle state) so all callers await the in-progress shutdown rather than treating it as complete.
if self.closed.swap(true, Ordering::AcqRel) {
return Ok(());
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/mod.rs:514
- An enqueue can clone this sender, increment both counters, and then race with
abort(): Tokio may drop the receiver, while shutdown resetstotal_bufferedto zero at line 687. Ifunbounded_sendthen fails, this rollback subtracts from zero and wraps the total tousize::MAX. Keep the sender lock held through the synchronous accounting and publish so shutdown cannot reset the counters until either the command is queued or the rollback is complete.
// Take a clone of the sender and release the lock at once, so a slow
// enqueue never blocks a close.
let sender = {
let guard = state.sender.lock().unwrap();
guard.as_ref().ok_or_else(Self::closed_error)?.clone()
};
// Count the event before the worker can see it. The worker decrements
// the counts as soon as the event reaches a terminal outcome, and a
// fast terminal path (an oversized event with a handler that returns at
// once) can run before this call returns. Counting afterwards lets that
// decrement reach zero first and wrap the counts to `usize::MAX`.
state.buffered.fetch_add(1, Ordering::AcqRel);
self.total_buffered.fetch_add(1, Ordering::AcqRel);
if sender.unbounded_send(command).is_err() {
// The worker never saw the event, so it never decrements for it.
state.buffered.fetch_sub(1, Ordering::AcqRel);
self.total_buffered.fetch_sub(1, Ordering::AcqRel);
return Err(Self::closed_error());
}
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/worker.rs:211
- The acknowledgement is sent before
PartitionWorkeris dropped, so on the std-thread runtimeabort()can receive it and return while this worker still ownssend_client(and therefore the producer connection) and the delivery handlers. This contradicts the shutdown guarantee atmod.rs:618-620and explains theArc::try_unwrapfailure path there. Drop the worker-owned fields before signaling completion.
// Tell the client that this worker holds nothing more. The client waits
// for this, so a close does not return while a worker still runs.
if let Some(stopped) = self.stopped.take() {
let _ = stopped.send(());
}
7eda3cc to
d127fdc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (6)
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/mod.rs:674
- On the standard runtime,
task.abort()only detaches the worker. Ifabort()starts while a worker is awaiting a user delivery handler, this acknowledgement cannot resolve until that arbitrary future completes and may hang forever; the retry policy does not bound handler duration. Make handler execution cancellation-aware (for example, select it against an explicit worker cancellation signal) soabort()can actually complete independently of a stuck handler.
// Wait for every worker to stop. A cancelled task drops its end of the
// channel, which resolves the receiver with an error, so this waits for
// the worker to finish or to be dropped, and never for both.
for acknowledgement in acknowledgements {
let _ = acknowledgement.await;
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/mod.rs:633
- This marks the client closed before shutdown completes, so a concurrent second
closeorabortreturnsOk(())immediately while the first call may still be sending, invoking handlers, and holding the connection. That violates both methods' completion guarantees. Calls that observe an in-progress shutdown should await a shared shutdown-completion result; only calls after completion should return immediately.
if self.closed.swap(true, Ordering::AcqRel) {
return Ok(());
}
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/mod.rs:308
- Without the
tokiofeature, this runtime call creates a dedicatedstd::thread(sdk/core/typespec_client_core/src/async_runtime/standard_runtime.rs:106-127). Because the loop eagerly starts one worker per partition, even idle clients consume one long-lived OS thread per partition, which does not scale to high-partition-count hubs. Please multiplex partition workers on a bounded/shared executor or start a bounded number lazily.
let task = get_async_runtime().spawn(Box::pin(worker.run()));
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/worker.rs:254
- On the supported non-Tokio runtime, polling
azure_core::sleepspawns a new OS thread that sleeps for the full duration (sdk/core/typespec_client_core/src/async_runtime/standard_runtime.rs:163-205). This timer is created for every new batch; when a batch fills early, dropping the future does not stop its sleeper thread. High-throughput configurations such as a buffer count of 1 can therefore create an unbounded burst of concurrent threads. Please use a shared/cancelable timer mechanism rather than creating one runtime sleep per batch.
if timer.is_none() {
*timer = Some(Box::pin(sleep(self.max_wait_time)));
}
sdk/eventhubs/azure_messaging_eventhubs/src/producer/mod.rs:473
get_sendereagerly runsensure_senderbefore enteringRecoverableSender::send, whose retry loop performs the same ensure. A retryable link-attach error can therefore return here immediately, contradicting this helper's guarantee thatErrmeans retries were exhausted or the error was non-retryable. Construct the recoverable sender directly so acquisition happens inside its retry loop.
let sender = self.connection.get_sender(path).await?;
sdk/eventhubs/azure_messaging_eventhubs/src/producer/buffered/send_client.rs:61
- This sender setup runs outside the send retry loop, and the worker turns any error from
max_message_sizeinto an immediate terminal failure for an already accepted event. A transient attach/recovery error can therefore drain the buffered queue as failures without applying the configured retry policy. Resolve the link maximum through the same retry/recovery path before reporting the event as terminally failed.
async fn max_message_size(&self, partition_id: &str) -> Result<u64> {
let path = self.partition_path(partition_id)?;
let sender = self.producer.ensure_sender(path).await?;
sender.max_message_size().await?.ok_or_else(|| {
Add BufferedProducerClient. The caller enqueues single events, and the client handles the bounded buffering, the partition assignment, the batching, the background publishing, the back pressure, the retries, the delivery reporting, the flushing, and the shutdown. One worker owns each partition and is the only reader of its queue, so the events keep the order that the caller enqueued them and only one send is active for a partition at a time. A slow partition does not stop the other partitions. The client reports every terminal outcome one time, through the handlers that the builder registers. A handler for failed batches is required, because a buffered send reports its failure after the enqueue call already returned. Unlike ProducerClient::send_batch, the buffered path reports an AMQP Modified or Released outcome as a delivery failure, because neither outcome means that the service durably stored the events. Extract the serialization core of EventDataBatch into an owned EventDataBatchInner, so a background worker can hold a batch without a borrow of the ProducerClient. The public explicit-batching API does not change. Add ProducerClient::send_batch_envelope, which returns the raw AMQP outcome. send_batch now calls it and keeps its current behavior. Add ErrorKind::SendNotAccepted for a Modified or Released outcome. The workers spawn through azure_core::async_runtime, and the partition buffers use an async-lock semaphore, so the change adds no new dependency and no production dependency on tokio.
…roducer The other buffered live tests authenticate with the recording credential. This test opens the client with an Event Hubs connection string, which matches the connection-string live tests of the producer, the consumer, and the round trip.
…able `AbortableTask::abort` does not cancel work on the standard thread runtime. It detaches the thread and lets the await return at once. This crate turns off the default features of `azure_core`, so `abort` could return while a worker was still sending, was still calling a delivery handler, and still held the connection. The worker also reached the end of its queue on that path, which is the same path as a graceful close, so it published the batch that the caller asked it to abandon. The worker now reads the abandon flag before each send and returns without one, and it tells the client through a channel when it stops. A close waits for that acknowledgement, so it never reports that it closed while a worker still runs. A runtime that drops the task cancels the channel, so the wait ends there as well. The client also counts an event before it publishes the command to the worker. The worker decrements the counts as soon as an event reaches a terminal outcome, and an oversized event with a fast handler could do that before the enqueue counted it, which wrapped the counts to `usize::MAX`.
The feature entry linked issue Azure#3600, which is an unrelated Storage issue. It now links this pull request. The error entry named `EventHubsError::SendNotAccepted`, but the variant is on `ErrorKind`. Both entries are shorter.
The connection-string test targeted partition `4`. The test resource provisions four partitions, with the IDs `0` to `3`, so the test failed at `get_partition_properties` before it published anything. The test now asks the Event Hub which partitions exist. There are more tests than partitions, so the test shares one. `receive_bodies` takes the prefix of the test and counts only the events that carry it, so a shared partition cannot make one test read the events of another.
The worker held the capacity permits of a batch while it awaited the delivery handler. The handler runs on the worker task, and the worker is the only thing that returns a permit. A handler that enqueued to the same partition therefore waited for a permit that only the worker could return, and the worker waited for the handler. With one permit for the partition, that is a deadlock. An event is at a terminal outcome once the send settles, and an oversized event is terminal as soon as the worker sees it. The counts and the permits now go back at that point, before the handler runs.
d127fdc to
b8bb617
Compare
Summary
azure_messaging_eventhubspublishes only explicit batches today. The caller builds a batch and sends it. This change addsBufferedProducerClient, which accepts single events and publishes them in the background. The client does the bounded buffering, the partition assignment, the batching, the back pressure, the retries, the delivery reports, the flush, and the shutdown. .NET, Java, JavaScript, and Python have an equivalent client. Rust and Go do not.Motivation
An application that publishes many events must build the batches itself, or pay one round trip for each event. An application that builds the batches also owns the partition assignment, the batch size limits, the retry handling, and the shutdown order. Each of these is easy to get wrong, and each of these is already in this crate.
Changes
BufferedProducerClientandBufferedProducerClientBuilder, withenqueue_event,enqueue_events,flush,close,abort,total_buffered_event_count, andbuffered_event_count. The builder hasopenandopen_with_connection_string.EnqueueEventOptions,SendBatchSucceededContext,SendBatchFailedContext, andErrorKind::SendNotAccepted.EventDataBatchinto an ownedpub(crate) EventDataBatchInner, so a background worker can hold a batch without a borrow of theProducerClient.EventDataBatch<'a>keeps its lifetime and calls the new type, so every public signature stays the same.ProducerClient::send_batch_envelope, which returns the AMQP outcome directly.send_batchnow calls it and keeps its current behavior.$cbslink for each connection, and two authorizations that overlapped made the service reject the second one withNotAllowed. The live tests here found it: this client starts one sender for each partition, and 7 of 32 events failed. This branch carried that fix while it was in review. The rebase onto main drops the duplicate commit, so the change comes from main now.The change adds no dependency. The workers start through
azure_core::async_runtime, and the partition buffers use anasync-locksemaphore. There is no production dependency on tokio, and no second retry, connection, or task-lifecycle system: the workers publish through the currentRecoverableSender.Delivery semantics
ModifiedandReleasedare failures. Neither outcome shows that the service kept the events, so the buffered path reports both withErrorKind::SendNotAccepted.ProducerClient::send_batchstill maps them toOk(()), so this change is additive.flushputs a barrier in the queue of each partition. An event enqueued behind the barrier does not delay the flush.closesends the buffered events and then releases the resources.abortdiscards them. A drop stops the background work and logs a warning.Points for the API review
Streamof results was not selected, because a slow or absent consumer either grows the memory without a bound or stops the workers.close()andabort(), and notclose(bool). Both take&selfand are idempotent, so an application can keep the client in anArc.ProducerClient::close(self)is different.enqueue_eventreturnsResult<()>, and not the buffered count that .NET and JavaScript return. A count from an enqueue can look like a delivery signal, andtotal_buffered_event_count()already gives it.EnqueueEventOptionsis not#[non_exhaustive], which matchesSendEventOptionsandEventDataBatchOptions. The two result contexts are#[non_exhaustive].PartitionResolver: round robin, and a Jenkins lookup3 hash of the partition key. The tests hold the hash port to the 12 values from the .NET test suite, so a key reaches the same partition as in the other SDKs.Validation
cargo fmt --check,cargo clippy --all-targets --all-features -- -D warnings,cargo test --all-features(186 unit tests and 54 doc tests, no failures), andRUSTDOCFLAGS='-Dwarnings' cargo docall pass. The Build Analyze gate also passes locally.39 of the unit tests cover the buffered producer. They run through a crate-internal
BufferedSendClientseam with a mock that gives prepared AMQP outcomes and per-call gates, so they need no network and no test uses a sleep to show a race. The full set ran 20 times in sequence with no failure.The live tests pass against a real Event Hub with 5 partitions: the enqueue and receive round trip, explicit partition routing, the partial-batch timeout, the graceful close, automatic partition assignment, the connection-string client, and the in-crate forced-error recovery test, which makes sure that a link failure loses no event and reports no event two times. The five Microsoft Entra ID tests ran through a temporary local change that authenticates with a connection string, because the test identity holds no
Sendrole on that namespace. Their committed form needs a run by someone whose identity has the role.Remaining gates
cargo run --manifest-path eng/tools/generate_api_report/Cargo.toml -- --package azure_messaging_eventhubs; the repository.gitignoreexcludesreview/, so this branch does not contain it.with_transport_typeto the producer and the consumer builder.BufferedProducerClientBuilderforwardsapplication_id,retry_options, andcustom_endpointtoProducerClient::builder(), and it needs the same forward for the transport type. Without that one method, a buffered producer cannot use AMQP over WebSockets. Neither change blocks the other, and the second one to merge adds the method.producer/batch.rs, and feat(eventhubs): support AMQP-over-WebSockets transport #4596 editsproducer/mod.rsand the changelog, which this change also edits. The second change to merge needs a small rebase.