Skip to content

Conversation

@mtmk
Copy link
Member

@mtmk mtmk commented Mar 26, 2025

if application is hanging on the message in foreach loop (as we are yielding) we stop the timer until iteration moved on to stop timer kicking in and issuing extra pull requests. This doesn't affect message delivery but increments consumer sequence unnecessarily. this behavior is now consistent with the Go client.

fixes #793

@mtmk mtmk requested a review from Copilot March 26, 2025 18:38
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses the issue of unnecessary pull requests by pausing the heartbeat timer while yielding messages.

  • Introduces calls to StopHeartbeatTimer and ResetHeartbeatTimer in asynchronous message iteration methods.
  • Adds a heartbeat timer control method in NatsJSConsume and duplicates the logic in NatsJSFetch.

Reviewed Changes

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

File Description
src/NATS.Client.JetStream/NatsJSConsumer.cs Inserts heartbeat timer control calls to prevent unnecessary pull requests.
src/NATS.Client.JetStream/Internal/NatsJSConsume.cs Adds StopHeartbeatTimer and modifies ResetHeartbeatTimer implementation.
src/NATS.Client.JetStream/Internal/NatsJSFetch.cs Duplicates StopHeartbeatTimer with an idle timeout check to match heartbeat logic.
Comments suppressed due to low confidence (2)

src/NATS.Client.JetStream/Internal/NatsJSConsume.cs:168

  • The StopHeartbeatTimer implementation in NatsJSConsume.cs does not check for an idle timeout (unlike NatsJSFetch.cs). Verify that this inconsistency in behavior is intentional to avoid unintended timer actions.
public void StopHeartbeatTimer() => _timer.Change(Timeout.Infinite, Timeout.Infinite);

src/NATS.Client.JetStream/Internal/NatsJSConsume.cs:170

  • ResetHeartbeatTimer in NatsJSConsume.cs resets both the due time and period to _hbTimeout, whereas the pattern in NatsJSFetch.cs uses Timeout.Infinite for the period. Confirm that this difference is deliberate to ensure consistent timer behavior.
public void ResetHeartbeatTimer() => _timer.Change(_hbTimeout, _hbTimeout);

Copy link
Contributor

@scottf scottf left a comment

Choose a reason for hiding this comment

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

LGTM

@mtmk mtmk merged commit bd12674 into main Apr 9, 2025
13 checks passed
@mtmk mtmk deleted the fix-consume-idle-timeout branch April 9, 2025 12:46
mtmk added a commit that referenced this pull request Apr 9, 2025
* Fix heartbeat timer to prevent unnecessary pulls (#794)
* Inbox Subscription Memory leak .NetStandard2.0 (#803)
* Fix reader buffer bug with typo (#799)
* Fix handling for OnNoData when ignoring deletes (#790)
@mtmk mtmk mentioned this pull request Apr 9, 2025
mtmk added a commit that referenced this pull request Apr 9, 2025
* Fix heartbeat timer to prevent unnecessary pulls (#794)
* Inbox Subscription Memory leak .NetStandard2.0 (#803)
* Fix reader buffer bug with typo (#799)
* Fix handling for OnNoData when ignoring deletes (#790)
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.

Redelivery on ack timeout always 30s

3 participants