Skip to content

feat: filter for ping events #12

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

feat: filter for ping events #12

wants to merge 3 commits into from

Conversation

infiniteregrets
Copy link
Member

@infiniteregrets infiniteregrets commented Mar 12, 2025

Important

Adds ping event handling and timeout detection in Stream.readStream() to ensure connection health.

  • Behavior:
    • Adds ping event handling in Stream.readStream() to update lastPingTime on 'ping' events.
    • Introduces PING_TIMEOUT_MS constant and checkPingTimeout function to detect ping timeouts.
    • Throws error if no ping received for over 30 seconds.
  • Error Handling:
    • Clears pingInterval on stream closure or error in Stream.readStream().
    • Throws pingTimeoutError if detected during stream processing.
  • Misc:
    • Adds pingInterval to periodically check for ping timeouts in Stream.readStream().

This description was created by Ellipsis for 648f827. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 648f827 in 2 minutes and 27 seconds

More details
  • Looked at 91 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. src/index.extras.ts:454
  • Draft comment:
    Reset pingTimeoutError on receiving a ping to clear previous timeout state.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    This comment is purely informative, explaining what a piece of code does. It doesn't provide a suggestion, ask for confirmation, or point out a potential issue.
2. src/index.extras.ts:71
  • Draft comment:
    Typo detected in the S2ClientConfig type definition: 'appedRetryPolicy' should likely be renamed to 'appendRetryPolicy' for clarity and consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. src/index.extras.ts:135
  • Draft comment:
    Typographical error in the S2Account class: the property 'accountURLSuffx' should be corrected to 'accountURLSuffix' for improved readability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. src/index.extras.ts:239
  • Draft comment:
    Typographical error in the S2Basin class: the property 'basinURLSuffx' should likely be renamed to 'basinURLSuffix' for consistency and clarity.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_aufrX3Waux95n4Wz


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

if (pingTimeoutError) throw pingTimeoutError;

if (event.event === 'ping') {
lastPingTime = Date.now();
Copy link

Choose a reason for hiding this comment

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

When handling a 'ping' event, reset the timeout error. Consider adding pingTimeoutError = null; after updating lastPingTime to clear any previous error.

Suggested change
lastPingTime = Date.now();
lastPingTime = Date.now(); pingTimeoutError = null;

@infiniteregrets infiniteregrets marked this pull request as draft March 12, 2025 20:27
Comment on lines +422 to +423
const PING_TIMEOUT_MS = 30000;
let lastPingTime = Date.now();
Copy link
Member

Choose a reason for hiding this comment

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

I think it should just be lastMessageTime (so also reset it on data). And timeout can be 20s to be consistent with other SDKs.

@@ -433,33 +444,47 @@ class Stream {
);
stream = response as EventStream<ReadResponse>;
if (!stream) return;

pingInterval = setInterval(checkPingTimeout, 1000);
Copy link
Member

Choose a reason for hiding this comment

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

This could be set much more accurately rather than waking up every second

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