Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix: avoid incrementing readErrCount for temporary errors and reset o…#1007

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
yangdm0209 wants to merge1 commit intogorilla:release-1.5
base:release-1.5
Choose a base branch
Loading
fromsandwich-go:release-1.5

Conversation

@yangdm0209
Copy link

@yangdm0209yangdm0209 commentedNov 10, 2025
edited
Loading

Commit title

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

Description

  • Do not incrementreadErrCount for temporary errors such as timeouts, as they do not indicate actual connection failures.
  • ResetreadErrCount after successful read operations to reflect the true health status of the connection.
  • This preventsreadErrCount 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 wherereadErrCount 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 causereadErrCount 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 theContributing guide
  • Read theCode of Conduct
  • Provided clear commit messages and code comments.
  • Added/updated documentation as needed.
  • Requested review from gorilla/pull-request-reviewers.

BeatieWolfe reacted with thumbs down emoji
…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 commentedNov 11, 2025
edited
Loading

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

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

ThereadErrCount 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 statementc.readErrCount = 0 is only executed whenc.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.

BeatieWolfe reacted with rocket emoji

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

Reviewers

No reviews

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yangdm0209@BeatieWolfe

[8]ページ先頭

©2009-2025 Movatter.jp