Skip to content

Conversation

@yangdm0209
Copy link

@yangdm0209 yangdm0209 commented Nov 10, 2025

Commit title

fix: avoid incrementing readErrCount for temporary errors and reset on successful read

Description

  • Do not increment readErrCount for temporary errors such as timeouts, as they do not indicate actual connection failures.
  • Reset readErrCount after successful read operations to reflect the true health status of the connection.
  • This prevents readErrCount from growing due to normal timeout logic and provides a more accurate error state.

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Go Version Update
  • Dependency Update

Description

This PR addresses an issue where readErrCount was incremented on all errors, including temporary errors such as timeouts. Since timeouts often occur as part of normal connection management, especially when read deadlines are used, counting them as errors could lead to misleading high error counts and unnecessary connection termination or misreporting.

Now, only permanent errors cause readErrCount to increase, and a successful read resets this counter, better reflecting the real stability of the connection.

Related Tickets & Documents

  • Related Issue #
  • Closes #

Added/updated tests?

  • Yes
  • No, and this is why: Please add details if you do not include tests (if the repo policy requires, plan test additions)
  • I need help with writing tests

Run verifications and test

  • make verify is passing
  • make test is passing

Checklist (before submitting):

  • Read the Contributing guide
  • Read the Code of Conduct
  • Provided clear commit messages and code comments.
  • Added/updated documentation as needed.
  • Requested review from gorilla/pull-request-reviewers.

…n successful read

- Do not increment readErrCount for temporary errors such as timeouts, as they do not indicate actual connection failures.
- Reset readErrCount after successful read operations to reflect the true health status of the connection.
- This prevents readErrCount from growing due to normal timeout logic and provides a more accurate error state.
@BeatieWolfe
Copy link

BeatieWolfe commented Nov 11, 2025

Although the underlying network connection may still be usable after returning an error, a websocket.Conn becomes unusable once its underlying connection encounters an error.

The readErr field reflects the health status of the websocket.Conn. It is set to a non-nil value when reads are no longer possible.

The readErrCount field tracks how many times the application has attempted to read from an unusable connection. Resetting this counter does not affect the actual health status of the connection.

None of the above prevents the normal use of a read deadline to detect when the peer is not sending data as expected.

Edit: Because the websocket.Conn does call read on the underlying connection after a read error is encountered, the statement c.readErrCount = 0 is only executed when c.readErrCount is zero. The actual effect of this PR is that a temporary error may propagate to the application. The PR does not do what OP thinks it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants