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

Add protection against concurrent Readers #391

Open
oslook opened this issue May 12, 2023 · 15 comments
Open

Add protection against concurrent Readers #391

oslook opened this issue May 12, 2023 · 15 comments
Milestone

Comments

@oslook
Copy link

oslook commented May 12, 2023

During my use of the library(server and client both v1.8.7), the server occasionally receives the following prompt,

failed to get reader: received header with unexpected rsv bits set: false:true:true

which seems to be related to this part of the code.

write.go L300:

	c.writeHeader.rsv1 = false
	if flate && (opcode == opText || opcode == opBinary) {
		c.writeHeader.rsv1 = true
	}

Could you please confirm if it is necessary to add the following code here?

	c.writeHeader.rsv1 = false
	c.writeHeader.rsv2 = false
	c.writeHeader.rsv3 = false
@vendion
Copy link

vendion commented Jun 25, 2023

I'm getting something slightly similar using the same version: failed to get reader: received header with unexpected rsv bits set: true:true:true

@soypat
Copy link

soypat commented Jul 23, 2023

Me too- I've checked with wireshark and the websocket frame is OK, it's just being parsed wrong on nhooyr/websocket's side.

@nhooyr
Copy link
Owner

nhooyr commented Sep 28, 2023

That's quite concerning that all three of you guys are seeing that... I'd suggest disabling compression though it'll be disabled by default in the next release.

I'll go through the code again to see if I can find the bug. The autobahn compression tests pass fine under the race detector so I'm surprised to see a bug like this.

@nhooyr nhooyr added the bug label Sep 28, 2023
@nhooyr
Copy link
Owner

nhooyr commented Sep 28, 2023

@soypat If possible, can you share the frames you checked with wireshark? Maybe I can replay them with the library and thus reproduce. That would help immensely in debugging.

This was referenced Sep 28, 2023
@nhooyr nhooyr added this to the v1.8.8 milestone Oct 13, 2023
@nhooyr
Copy link
Owner

nhooyr commented Oct 13, 2023

There's been enough changes for v1.8.8 that whatever this was is likely fixed. Compression in particular is disabled by default now and I recommend keeping it that way. Let me know if you see it again once v1.8.8 is released.

@nhooyr nhooyr closed this as completed Oct 13, 2023
nhooyr added a commit that referenced this issue Oct 19, 2023
For #395

Somehow currently reproduces #391...

Debugging still.
@nhooyr
Copy link
Owner

nhooyr commented Oct 19, 2023

Nvm I was able to reproduce, see 249edb2

@nhooyr nhooyr reopened this Oct 19, 2023
@nhooyr
Copy link
Owner

nhooyr commented Oct 19, 2023

Were you guys using the library through a proxy?

@nhooyr
Copy link
Owner

nhooyr commented Oct 19, 2023

Nvm, that bug was from my proxy not working correctly. Fixed now.

@nhooyr
Copy link
Owner

nhooyr commented Oct 19, 2023

Actually nvm, this is probably caused by #355 so I'll close after fixing that.

@nhooyr nhooyr reopened this Oct 19, 2023
@nhooyr
Copy link
Owner

nhooyr commented Oct 19, 2023

Done so closing.

@nhooyr nhooyr closed this as completed Oct 19, 2023
@maggie44
Copy link
Contributor

maggie44 commented Nov 2, 2023

I still see this error on occasions. Seen it today on the latest release: failed to get reader: received header with unexpected rsv bits set: true:true:false"

Whenever I see it, it so far has always been the result of me having two readers open at the same time. It states in the library that Only one Reader may be open at a time, but sometimes an error in my code where I have two readers causes the issue.

Not sure if the error message could be clearer for diagnosis. Adding this quick comment here in case anyone comes across this issue and finds this thread.

@nhooyr
Copy link
Owner

nhooyr commented Nov 3, 2023

Ah yea that would do it. I'll add a link to this thread in that error message.

Could also add some protection here perhaps without much performance cost.

@nhooyr nhooyr reopened this Nov 3, 2023
@nhooyr nhooyr changed the title received header with unexpected rsv bits set: false:true:true Add protection against concurrent Readers Nov 3, 2023
@nhooyr nhooyr modified the milestones: v1.8.8, v1.9.0 Nov 3, 2023
@maggie44
Copy link
Contributor

maggie44 commented Nov 3, 2023

I wonder if there is also a way to have two readers, where both (or multiple) receive the same message. This would be helpful for me in a number of scenarios. Or maybe when initiating one reader it closes the other. I am already passing the ws connection around, but ideally wouldn't have to pass around the reader, and could just close one and open it up elsewhere.

@nhooyr
Copy link
Owner

nhooyr commented Nov 3, 2023

There shouldn't be any scenario where you want to do a concurrent read on the connection. There should only ever be one goroutine reading from the connection. Can you clarify why you feel you need multiple readers or even why you're passing the reader around like that? Perhaps an example in code?

@maggie44
Copy link
Contributor

maggie44 commented Nov 3, 2023

On the multiple reader, makes sense. Closing and reopening or passing around, I read in a loop but some messages call a series of functions and those functions need to take over the connection and receive all future messages after their call. For those functions I need to either stop the loop and move the reader down or relay all messages through a channel on through another connection.

Hard to fully explain, maybe at some point it will hit me how best to do it. Didn’t want to hijack the thread though, will think on it.

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

No branches or pull requests

5 participants