fix#27
Conversation
WalkthroughThis release introduces validation requiring a minimum Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/AzureEventHub.cs (1)
86-97:⚠️ Potential issue | 🟠 MajorMajor:
Consumer.MaximumWaitTimenow doubles as an outer-loop idle timeout, which is an undocumented behavior change and can cause premature exit.Two concerns with the new stop condition:
Undocumented semantic overload. Previously
Consumer.MaximumWaitTimeonly influenced the SDK's internal wait behavior (passed toEventProcessorClientOptions.MaximumWaitTime). Now it also terminates the outer loop ifDateTime.UtcNow - lastEventTime >= MaximumWaitTime. Users who setMaximumWaitTimeto, say, 10 seconds intending only to bound SDK polling will now see the Task exit after 10 idle seconds — a behavior change. The XML docs inDefinitions/Consumer.cs(lines 86–94) still describe only the old role. Please update them and add a note to the CHANGELOG.
lastEventTimeis initialized before processing starts.lastEventTime = DateTime.UtcNowruns at line 48, beforeCreateBlobContainerClient, optional container creation,CreateEventProcessorClient, andStartProcessingAsyncon lines 74–84. If any of those (especiallyCreateIfNotExistsAsyncon a slow storage account) takes longer thanMaximumWaitTime, the outer loop exits immediately on the first iteration without ever receiving an event. ResetlastEventTimeimmediately afterStartProcessingAsyncsucceeds (or the first time the outer loop runs) to anchor the idle window to "time since processing actually began".🛠️ Suggested fix
await processorClient.StartProcessingAsync(cancellationToken); + lastEventTime = DateTime.UtcNow; while (!stopProcessing) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/AzureEventHub.cs` around lines 86 - 97, The outer-loop stop condition incorrectly reuses Consumer.MaximumWaitTime as an idle-exit timeout and uses lastEventTime initialized before setup, causing premature exits; update the XML docs in Definitions/Consumer.cs and CHANGELOG to document the behavior change or note that MaximumWaitTime is only for the SDK (preferred), then modify the loop logic so it does NOT treat MaximumWaitTime as the outer-loop idle timeout (remove the DateTime.UtcNow - lastEventTime check) or, if you intend to keep the idle-exit feature, reinitialize lastEventTime immediately after StartProcessingAsync succeeds (or on the first loop iteration) to anchor the idle window to the actual start of processing; locate symbols: Consumer.MaximumWaitTime, lastEventTime, CreateBlobContainerClient, CreateIfNotExistsAsync, CreateEventProcessorClient, and StartProcessingAsync to implement and document the chosen fix.
🧹 Nitpick comments (3)
Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/Definitions/Options.cs (1)
20-20: Nit: include the unit in the minimum-value note.The preceding line says "in seconds" but the constraint line doesn't, which can be misread once quoted out of context (e.g., in generated docs or validation error text).
📝 Suggested wording
- /// NOTE: Must be at least 0.1 + /// NOTE: Must be at least 0.1 seconds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/Definitions/Options.cs` at line 20, The comment for the minimum value in Options.cs is ambiguous—update the note next to the relevant property (the "NOTE: Must be at least 0.1" line) to include the unit "seconds" (e.g., "NOTE: Must be at least 0.1 seconds") so it clearly matches the preceding "in seconds" description and prevents misinterpretation in docs or validation messages.Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/AzureEventHub.cs (2)
48-56: Minor:lastEventTimeis read/written from different threads without synchronization.
lastEventTimeis written fromProcessEventHandler(SDK callback thread) and read from the outer loop on line 90.DateTimeis an 8-byte struct and the CLR does not guarantee atomic reads/writes of 64-bit values on all platforms — a torn read is theoretically possible, which could make the idle-timeout check use a garbage value. On x64 this is in practice atomic, so impact is low, but consider switching toInterlocked.Exchangeonlongticks, or marking both accesses withVolatile.Read/Volatile.Writeof the ticks.🔒 Sketch
- var lastEventTime = DateTime.UtcNow; + long lastEventTicks = DateTime.UtcNow.Ticks; ... - lastEventTime = DateTime.UtcNow; + Interlocked.Exchange(ref lastEventTicks, DateTime.UtcNow.Ticks); ... - (maximumWaitTime.HasValue && - DateTime.UtcNow - lastEventTime >= - maximumWaitTime.Value)) + (maximumWaitTime.HasValue && + DateTime.UtcNow - new DateTime(Interlocked.Read(ref lastEventTicks), DateTimeKind.Utc) >= + maximumWaitTime.Value))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/AzureEventHub.cs` around lines 48 - 56, The field local variable lastEventTime is accessed from different threads (written inside ProcessEventHandler and read in the outer idle-timeout loop) and may suffer torn 64-bit reads; change lastEventTime from DateTime to a long representing DateTime.UtcNow.Ticks and use Interlocked.Exchange to set it in ProcessEventHandler and Interlocked.Read (or Volatile.Read) to read it in the idle-checking loop, converting ticks back to DateTime when needed (reference symbols: lastEventTime, ProcessEventHandler, and the idle-timeout loop that reads lastEventTime).
39-40: Optional: preferArgumentOutOfRangeExceptionfor parameter validation.
throw new Exception(...)for input validation is an anti-pattern — it forces callers to catch the base type and loses semantic meaning. The rest of this method already throws bareExceptionfor validation (lines 35, 37), so this is consistent in-file, but consider migrating all three toArgumentException/ArgumentOutOfRangeExceptionin a follow-up.- if (options.ConsumeAttemptDelay < 0.1) - throw new Exception("Options.ConsumeAttemptDelay must be at least 0.1 seconds."); + if (options.ConsumeAttemptDelay < 0.1) + throw new ArgumentOutOfRangeException( + nameof(options.ConsumeAttemptDelay), + options.ConsumeAttemptDelay, + "Options.ConsumeAttemptDelay must be at least 0.1 seconds.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/AzureEventHub.cs` around lines 39 - 40, Replace the generic Exception thrown for the ConsumeAttemptDelay validation with an ArgumentOutOfRangeException: in the method that checks options.ConsumeAttemptDelay, throw new ArgumentOutOfRangeException("ConsumeAttemptDelay", "Options.ConsumeAttemptDelay must be at least 0.1 seconds.") (or use nameof(ConsumeAttemptDelay) if accessible) so callers receive a semantically correct error type for invalid range; update the throw in the block that currently reads throw new Exception("Options.ConsumeAttemptDelay must be at least 0.1 seconds.").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Frends.AzureEventHub.Receive/CHANGELOG.md`:
- Around line 3-8: Update the 2.7.0 changelog entry to explicitly list the two
functional changes: (1) runtime validation now rejects ConsumeAttemptDelay
values < 0.1 — mark this as a potential breaking change and add an upgrade note
advising users to set ConsumeAttemptDelay >= 0.1, and (2) the event-loop stop
behavior changed so MaxRunTime expiry is checked in the outer loop and
Consumer.MaximumWaitTime also acts as an idle timeout that can terminate the
outer loop when no events arrive; explain how this affects Task exit timing;
also remove the trailing whitespace on the existing line with the short-delay
note.
---
Outside diff comments:
In `@Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/AzureEventHub.cs`:
- Around line 86-97: The outer-loop stop condition incorrectly reuses
Consumer.MaximumWaitTime as an idle-exit timeout and uses lastEventTime
initialized before setup, causing premature exits; update the XML docs in
Definitions/Consumer.cs and CHANGELOG to document the behavior change or note
that MaximumWaitTime is only for the SDK (preferred), then modify the loop logic
so it does NOT treat MaximumWaitTime as the outer-loop idle timeout (remove the
DateTime.UtcNow - lastEventTime check) or, if you intend to keep the idle-exit
feature, reinitialize lastEventTime immediately after StartProcessingAsync
succeeds (or on the first loop iteration) to anchor the idle window to the
actual start of processing; locate symbols: Consumer.MaximumWaitTime,
lastEventTime, CreateBlobContainerClient, CreateIfNotExistsAsync,
CreateEventProcessorClient, and StartProcessingAsync to implement and document
the chosen fix.
---
Nitpick comments:
In `@Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/AzureEventHub.cs`:
- Around line 48-56: The field local variable lastEventTime is accessed from
different threads (written inside ProcessEventHandler and read in the outer
idle-timeout loop) and may suffer torn 64-bit reads; change lastEventTime from
DateTime to a long representing DateTime.UtcNow.Ticks and use
Interlocked.Exchange to set it in ProcessEventHandler and Interlocked.Read (or
Volatile.Read) to read it in the idle-checking loop, converting ticks back to
DateTime when needed (reference symbols: lastEventTime, ProcessEventHandler, and
the idle-timeout loop that reads lastEventTime).
- Around line 39-40: Replace the generic Exception thrown for the
ConsumeAttemptDelay validation with an ArgumentOutOfRangeException: in the
method that checks options.ConsumeAttemptDelay, throw new
ArgumentOutOfRangeException("ConsumeAttemptDelay", "Options.ConsumeAttemptDelay
must be at least 0.1 seconds.") (or use nameof(ConsumeAttemptDelay) if
accessible) so callers receive a semantically correct error type for invalid
range; update the throw in the block that currently reads throw new
Exception("Options.ConsumeAttemptDelay must be at least 0.1 seconds.").
In
`@Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/Definitions/Options.cs`:
- Line 20: The comment for the minimum value in Options.cs is ambiguous—update
the note next to the relevant property (the "NOTE: Must be at least 0.1" line)
to include the unit "seconds" (e.g., "NOTE: Must be at least 0.1 seconds") so it
clearly matches the preceding "in seconds" description and prevents
misinterpretation in docs or validation messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 677eb32f-f4a3-4912-8a28-630e3c95fab0
📒 Files selected for processing (4)
Frends.AzureEventHub.Receive/CHANGELOG.mdFrends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/AzureEventHub.csFrends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/Definitions/Options.csFrends.AzureEventHub.Receive/Frends.AzureEventHub.Receive/Frends.AzureEventHub.Receive.csproj
| ## [2.7.0] - 2026-04-26 | ||
|
|
||
| ### Fixed | ||
|
|
||
| - Resolve the problem with short delays between checks | ||
|
|
There was a problem hiding this comment.
Expand the 2.7.0 entry — current note doesn't describe the actual changes or potential breaking behavior.
Two functional changes land in this release, neither of which is captured by "Resolve the problem with short delays between checks":
- A new runtime validation rejects
ConsumeAttemptDelay < 0.1. This is a behavior change and potentially a breaking change for any caller that previously configured a smaller value — it should have an upgrade note. - The event-loop stop condition changed:
MaxRunTimeexpiry is now evaluated in the outer loop, andConsumer.MaximumWaitTimenow also acts as an idle timeout that terminates the outer loop when no events arrive within that window. This alters when the Task exits and should be documented.
Also, line 7 has trailing whitespace.
As per coding guidelines: "Include all functional changes and indicate breaking changes with upgrade notes. Avoid notes like 'refactored xyz' unless it affects functionality."
📝 Suggested rewording
## [2.7.0] - 2026-04-26
-
-### Fixed
-
-- Resolve the problem with short delays between checks
+
+### Changed
+
+- `Options.ConsumeAttemptDelay` now enforces a minimum of `0.1` seconds; values below this throw at runtime.
+ Upgrade note: review pipelines that set a smaller delay and raise the value to at least `0.1`.
+- Event-loop stop conditions reworked: `Options.MaxRunTime` is now checked in the outer loop, and
+ `Consumer.MaximumWaitTime` additionally terminates the loop if no event is received within that window.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Frends.AzureEventHub.Receive/CHANGELOG.md` around lines 3 - 8, Update the
2.7.0 changelog entry to explicitly list the two functional changes: (1) runtime
validation now rejects ConsumeAttemptDelay values < 0.1 — mark this as a
potential breaking change and add an upgrade note advising users to set
ConsumeAttemptDelay >= 0.1, and (2) the event-loop stop behavior changed so
MaxRunTime expiry is checked in the outer loop and Consumer.MaximumWaitTime also
acts as an idle timeout that can terminate the outer loop when no events arrive;
explain how this affects Task exit timing; also remove the trailing whitespace
on the existing line with the short-delay note.
2 participants
Summary by CodeRabbit