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

fix: use atomic.bool to ensure atomic operation of isWriting #870

Closed

Conversation

rfyiamcool
Copy link
Contributor

@rfyiamcool rfyiamcool commented Nov 21, 2023

summary

I think isWriting as bool type is not thread-safe operation under concurrency. 😁

For example, multiple Goroutines determine whether iswriting is false before writing data, and if isWriting is false, write data. but, the two operations of querying isWriting and setting isWriting are not atomic.

example code

the following scene

goroutine 1

func (w *messageWriter) flushFrame(final bool, extra []byte) error {
	...
	
	if c.isWriting {
		panic("concurrent write to websocket connection")
	}

 	// run here 😁

	c.isWriting = true

        c.write(....)

        ....
}

goroutine 2

func (w *messageWriter) flushFrame(final bool, extra []byte) error {
	...
	if c.isWriting {
		panic("concurrent write to websocket connection")
	}

 	// run here 😁

	c.isWriting = true

        c.write(....)

        ....
}

@FZambia
Copy link
Contributor

FZambia commented Nov 21, 2023

Hello, as written in comment for isWriting - it's a best effort protection from concurrent writes. If you have a race here it's a signal you are writing concurrently - thus a signal you are using Gorilla WebSocket API in a wrong way. It does not support concurrent writes. So for me it seems to be working as intended.

@ghost
Copy link

ghost commented Nov 21, 2023

I agree with FZambia's comment. Here's some more information:

The current code is similar to the check for concurrent writes in Go maps. The Go map check uses a bit flag, not an atomic bool (see uses of hashWriting in runtime/map.go).

The websocket package documentation says:

Connections support one concurrent reader and one concurrent writer.

There is not a race when the application uses the API per the documentation.

@rfyiamcool
Copy link
Contributor Author

I agree with FZambia's comment. Here's some more information:

The current code is similar to the check for concurrent writes in Go maps. The Go map check uses a bit flag, not an atomic bool (see uses of hashWriting in runtime/map.go).

The websocket package documentation says:

Connections support one concurrent reader and one concurrent writer.

There is not a race when the application uses the API per the documentation.

You are right. 😁 @pennystevens


image

@BroadbentJim
Copy link

Might be worth closing / rejecting this PR.

@rfyiamcool rfyiamcool closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants