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

[question] Question on concurrent writer. #596

Closed
linrl3 opened this issue Jun 1, 2020 · 10 comments
Closed

[question] Question on concurrent writer. #596

linrl3 opened this issue Jun 1, 2020 · 10 comments
Labels

Comments

@linrl3
Copy link

linrl3 commented Jun 1, 2020

Describe the problem you're having

A clear and concise description of what the bug is.
From godoc:Connections support one concurrent reader and one concurrent writer. Applications are responsible for ensuring that no more than one goroutine calls the write methods (NextWriter, SetWriteDeadline, WriteMessage, WriteJSON, EnableWriteCompression, SetCompressionLevel) concurrently and that no more than one goroutine calls the read methods (NextReader, SetReadDeadline, ReadMessage, ReadJSON, SetPongHandler, SetPingHandler) concurrently.

So my problem is could we use 2 or more goroutine for Write method? I used 2 goroutine without any mutex, but it looked all good.

Versions

Go version: go version

package version: run git rev-parse HEAD inside the repo

"Show me the code!"

A minimal code snippet can be useful, otherwise we're left guessing!

Hint: wrap it with backticks to format it

@linrl3 linrl3 added the question label Jun 1, 2020
@fishyww
Copy link

fishyww commented Jun 12, 2020

Hello ~ can you provide sample code?

@ghost
Copy link

ghost commented Jun 18, 2020

So my problem is could we use 2 or more goroutine for Write method?

No, the documentation says it's not allowed. Use a mutex or some other form of synchronization.

I used 2 goroutine without any mutex, but it looked all good.

You were lucky.

@linrl3
Copy link
Author

linrl3 commented Jun 22, 2020

So my problem is could we use 2 or more goroutine for Write method?

No, the documentation says it's not allowed. Use a mutex or some other form of synchronization.

I used 2 goroutine without any mutex, but it looked all good.

You were lucky.

Yes, I got panic eventually...

@melekes
Copy link

melekes commented Sep 1, 2020

This can be closed imho.

@justinrush
Copy link

I get why you can't support more than one concurrent writer and don't have any real problem using a mutex, but since you specifically don't allow more than one writer, why not just have the mutex inside of the websocket struct? why force every person that uses the code to implement the mutex in their application?

@ghost
Copy link

ghost commented Oct 30, 2020

@justinrush A mutex is not needed when a single goroutine writes to the connection. None of the examples in this repository use a mutex.

There's not a single method where the mutex should be locked/unlocked. In the following code, the lock comes before setting deadline and after closing the writer. I think it would have been better to design the API so the connection can manage the lock, but it's too late for that.

x.mu.Lock()
x.c.SetWriteDeadline(time.Now().Add(writeWait))
w, err := x.c.NextWriter(websocket.TextMessage)
if err != nil {
    return err
}
                      
...
w.Write(data)
...

err := w.Close()
x.mu.Unlock()
return err

@justinrush
Copy link

Ya, it just feels like its pretty common to have an app design where you end up needing the mutex. It makes sense that the ship has sailed on including it by default in the library as there are a bunch of projects that are already doing the locking. Thanks for following up

@ghost
Copy link

ghost commented Oct 30, 2020

@justinrush It may be common to use a mutex, but it's a lot easier to avoid server stalls on dead or stuck clients using a goroutine. Most users of mutexes should probably be using a goroutine to ensure no more than one concurrent writer.

@IngCr3at1on
Copy link

Without attempting to read garyburd's mind I would assume the reasoning here is simple; the package is meant to implement the websocket RFC, without attempting to assume how it will be used. Put even more simply; application logic belongs in the application.

@garyburd
Copy link
Contributor

@IngCr3at1on and the ghost answered all of the questions in this issue.

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

No branches or pull requests

6 participants