gorilla / websocket

Package gorilla/websocket is a fast, well-tested and widely used WebSocket implementation for Go.
https://gorilla.github.io
BSD 2-Clause "Simplified" License
22.52k stars 3.49k forks source link

[BUG] websocket on compress close duplicate #859

Open t0jc opened 1 year ago

t0jc commented 1 year ago

Is there an existing issue for this?

Current Behavior

in this line https://github.com/gorilla/websocket/blob/ac0789be11725ab2285233e9a3800c2312cff4fc/compression.go#L138

and this line https://github.com/gorilla/websocket/blob/ac0789be11725ab2285233e9a3800c2312cff4fc/conn.go#L1024

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

DVKunion commented 10 months ago

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

davidnewhall commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

jaitaiwan commented 9 months ago

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 commented 9 months ago

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 commented 7 months ago

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

jaitaiwan commented 7 months ago

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

elepedus commented 7 months ago

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

jaitaiwan commented 7 months ago

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 commented 7 months ago

Thank you! 🙏

conbanwa commented 4 months ago

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

hulkingshtick commented 3 months ago

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

jaitaiwan commented 2 months ago

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