Skip to content
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

Remove hideTempErr to allow downstream users to check for errors like net.ErrClosed #894

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

UnAfraid
Copy link
Contributor

@UnAfraid UnAfraid commented Feb 7, 2024

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

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

Description

Since this change https://github.com/gorilla/websocket/pull/840/files#diff-4f427d2b022907c552328e63f137561f6de92396d7a6e8f6c2ea1bcf0db52654L190-R197 we can no longer determinate if the errors coming from ReadMessage() are net.ErrClosed for example
Hardcoding the error message is not great option because it may vary from OS to OS and system locale

Related Tickets & Documents

Added/updated tests?

  • Yes
  • No, and this is why: please replace this line with details on why tests
    have not been included
  • I need help with writing tests

Run verifications and test

  • make verify is passing
  • make test is passing

@StevenACoffman
Copy link

@AlexVulaj This would fix several issues with the 1.5.1 release. Please take a look. Thanks!

@UnAfraid UnAfraid changed the title Remove hideTempErr to allow external users to check for errors like net.ErrClosed Remove hideTempErr to allow downstream users to check for errors like net.ErrClosed Feb 12, 2024
jaitaiwan
jaitaiwan previously approved these changes Feb 13, 2024
Copy link

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

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

LGTM, one small nit. I am a little cautious about this change because I haven't looked at the context of c.readErr. If other Members are happy then I don't see any structural issues with this.

conn_test.go Outdated Show resolved Hide resolved
@AlexVulaj
Copy link
Member

I'm happy to merge this @jaitaiwan , feel free to make a final call on the "t.Parallel" comment but I don't have any strong feelings.

@UnAfraid
Copy link
Contributor Author

Please let me know if t.Parallel() should be used and i can bring it back, i don't mind either way.

Copy link

@jaitaiwan jaitaiwan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d293aa5) 71.30% compared to head (52188df) 71.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #894      +/-   ##
==========================================
+ Coverage   71.30%   71.38%   +0.08%     
==========================================
  Files          11       11              
  Lines        1589     1583       -6     
==========================================
- Hits         1133     1130       -3     
+ Misses        349      347       -2     
+ Partials      107      106       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@StevenACoffman
Copy link

StevenACoffman commented Feb 15, 2024

Security and lint failures are unrelated to this change.

@jaitaiwan jaitaiwan merged commit 695e909 into gorilla:main Feb 15, 2024
9 of 12 checks passed
@jaitaiwan
Copy link

Agreed. Thanks for the contribution!

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.

None yet

4 participants