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

[BUG] websocket on compress close duplicate #859

Open
1 task done
t0jc opened this issue Oct 30, 2023 · 15 comments
Open
1 task done

[BUG] websocket on compress close duplicate #859

t0jc opened this issue Oct 30, 2023 · 15 comments
Labels

Comments

@t0jc
Copy link

t0jc commented Oct 30, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

in this line

if err := r.Close(); err != nil {

and this line

websocket/conn.go

Line 1024 in ac0789b

if err := c.reader.Close(); err != nil {

Expected Behavior

if close duplicate will report this log

2023/10/30 21:56:02 websocket: discarding reader close error: io: read/write on closed pipe

Steps To Reproduce

  1. create websocket link with "EnableCompression"
  2. use this websocket write someting
  3. in server side got log "websocket: discarding reader close error: io: read/write on closed pipe"

Anything else?

2023/10/30 21:56:02 websocket: discarding reader close error: io: read/write on closed pipe

@t0jc t0jc added the bug label Oct 30, 2023
@DVKunion
Copy link

the same +1, any solutions or temporary repair suggestions? :)

@davidnewhall
Copy link

davidnewhall commented Feb 9, 2024

Disabling compression does not get rid of the error. The error seems triggered by keep alives because it happens without any other requests. I sure wish I could pass in a custom logger for this so I can tune the error and figure out which thread it comes from.

@jaitaiwan
Copy link
Member

What would really help with moving forward with this bug is having reproduction steps. Then we can just run a go debugger and poke at the stacks.

@davidnewhall
Copy link

davidnewhall commented Feb 10, 2024

Sounds a bit beyond my skill, but I'd be happy to work with someone to figure this out.

I have an app that reproduces this error consistently. I have thousands of users, and I'd like to get this figured out before I release an app update with v1.5.1 of this library (which adds the new log message). If you download the app, you'll also need a (free) account at https://notifiarr.com. There's no (email or anything) verification so you can remain 100% anonymous there.

I'll try to get a reproducible setup using just mulery. Notifiarr users this code for the web socket, and it's much leaner. I'm 100% expecting the "bug" to be in my code, but I really don't know how to find it.

Looks like this error print has been removed. That's interesting and makes me think we're overlooking a bug somewhere.

@ghost
Copy link

ghost commented Feb 12, 2024

This issue (along with others) was introduced in commit 666c197. The commit message gives no reason for the change. Fix by reverting the changes around the lines of code linked in the issue.

@jaitaiwan
Copy link
Member

Thanks for the feedback folks. We recognise that we made a mistake in merging some of these changes and we're working on getting new PRs written and reviewed to fix this going forward.

@DVKunion
Copy link

Disabling compression does not get rid of the error. The error seems triggered by keep alives because it happens without any other requests. I sure wish I could pass in a custom logger for this so I can tune the error and figure out which thread it comes from.

Yes, this is just a temporary solution. I have abandoned comrpress to ensure that my logs have no erroneous outputs.If there is a better fix method, please remind me :)

@elepedus
Copy link

elepedus commented May 2, 2024

Did anyone manage to resolve this issue? We're using the library to proxy websocket connections, and would love a fix 🙏

@jaitaiwan
Copy link
Member

We’ve done a revert and plan to do a temporary release to cover things I believe .

@elepedus
Copy link

elepedus commented May 2, 2024

Oh awesome! When do you expect to release the fixed version?

@jaitaiwan
Copy link
Member

We’re talking about internally how to version it, once the decision is made we’ll do it straight away. In the meantime master represents that future version.

@elepedus
Copy link

elepedus commented May 3, 2024

Thank you! 🙏

@conbanwa
Copy link

same issue on @1.5.1, and is was just ok on @1.5.0 version

@hulkingshtick
Copy link

Was this fixed? There are no calls to the log package on the main branch.

@jaitaiwan
Copy link
Member

I believe so - v1.5.3 should be safe now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

7 participants