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 concurrent panic #67
Conversation
Why remove the pointer of |
I only wanted to pr this commit: lesismal@e21886f |
It's not removing the pointer of Plz see the 1st floor, the close() doesn't guard the process of changing the conn's state and closing the output chan, some other goroutine may push data to the output chan after the chan has been closed. And run the testing code, you will reproduce the panic. |
@lesismal The problem you are referring to does exist and you fixed it. |
@iwctwbai here |
Yes, I just happened to see your code and was a little confused @lesismal |
@iwctwbai Hope everything is fine with olahol. |
Hope everything is fine with olahol. |
@lesismal Hello, I have an question about your commit content is not close output channel. my soluation is defer a recover func in writeMessage. |
Because select {
case s.output <- message:
default:
s.melody.errorHandler(s, errors.New("session message buffer is full"))
} The chan will be gc even it is not closed. So, it's ok. |
Thank you for the fix 👍 Sorry for ghosting for so long |
Really happy to see u again, welcome back! |
Thank you for the kind words and the help fixing this old library 🙂 |
if two goroutines exec these two lines nearly the same time:
https://github.com/olahol/melody/blob/master/session.go#L59
https://github.com/olahol/melody/blob/master/session.go#L24
the two goroutines will get the same opening state, and if
close(s.output)
exec firsthttps://github.com/olahol/melody/blob/master/session.go#L63
case s.output <- message
will panic:https://github.com/olahol/melody/blob/master/session.go#L30
try this example to reproduce this bug:
then wait for enough time, we will get: