#117 - fix data races #119
Conversation
tot/socket/mock.socketer_test.go
Outdated
| @@ -19,7 +19,8 @@ func NewMock(socketURL *url.URL) *Socket { | |||
| commandIDMux: &sync.Mutex{}, | |||
| commands: NewCommandMap(), | |||
| handlers: NewEventHandlerMap(), | |||
| mux: &sync.Mutex{}, | |||
| connMux: &sync.Mutex{}, | |||
There was a problem hiding this comment.
I don't think we need more than muxer
tot/socket/socket.conner.go
Outdated
| socket.mux.Lock() | ||
| defer socket.mux.Unlock() | ||
| socket.connMux.Lock() | ||
| defer socket.connMux.Unlock() | ||
|
|
||
| if socket.connected { |
There was a problem hiding this comment.
Actually, My idea is
/*
Connected returns whether a connection exists.
Connected is a Conner implementation.
*/
func (socket *Socket) Connected() bool {
socket.stateMux.Lock()
defer socket.stateMux.Unlock()
return socket.connected
}
// SetConnected updates connected status
func (socket *Socket) setConnectedState(isConnected bool) {
socket.stateMux.Lock()
defer socket.stateMux.Unlock()
socket.connected = isConnected
}
// IsListening check whether the socket is listening
func (socket Socket) IsListening() bool {
socket.stateMux.Lock()
defer socket.stateMux.Unlock()
return socket.listening
}
// setListeningState update listening status
func (socket Socket) setListeningState(isListening bool) {
socket.stateMux.Lock()
defer socket.stateMux.Unlock()
socket.listening = isListening
}
There was a problem hiding this comment.
I think that adds unnecessary complexity for a simple piece of code, but I agree the connected property does the same thing. I can use the value of the conn instead and get rid of the connected property entirely.
|
There are some confusions on the code. What is purpose of |
|
A lot of logging the good for debug, But it make the code a bit harder for reading. |
tot/socket/socket.socketer.go
Outdated
| @@ -345,6 +351,7 @@ func (socket *Socket) listen() error { | |||
| socket.handleUnknown(response) | |||
| } | |||
|
|
|||
| socket.listenMux.Lock() | |||
| if !socket.listening { | |||
There was a problem hiding this comment.
Should break the loop by using channel
Example:
for {
select {
case <-quit:
break
default:
// Continue loop
}
}
There was a problem hiding this comment.
I agree, I'll make that change.
|
The The change I'm making to use the |
|
So these changes are fixing the data races in the tests, but there's still an issue with stoping the process, it's getting hung on a channel with no listener. I'll investigate further when I have a moment. |
|
I think the pain point is how you using muxer. It’s might create a lot of deadlocks in the lib |
|
I agree, I think I'm going to review the entire setup. |
tot/mock.socket_test.go
Outdated
| func (socket *MockSocket) Stop() error { | ||
| return nil | ||
| func (socket *MockSocket) Stop() { | ||
| return |
tot/socket/socket.conner.go
Outdated
| } | ||
| return socket.listenErr | ||
| return |
tot/socket/mock.chrome_test.go
Outdated
| case dataChan <- <-dataFeed: | ||
| } | ||
| } | ||
| return nil |
tot/socket/mock.chrome_test.go
Outdated
| chrome.logger.Debug("mock data written to mock chrome websocket") | ||
| } | ||
| } | ||
| return nil |
Change type
What types of changes does your code introduce to
go-chrome?Fixing data race issues in test mocks and API.
Checklist
What does this implement/fix? Please explain these changes.
Fixes a longstanding issue with data races in the library
net/http/httptest-raceto the CI test flagsThese changes cause the Ci builds to take about 3x longer (~10 minutes for
tipbuilds, ~3 minutes for other builds), I'll review the throttling behavior separately.Does this close any currently open issues?
#117
Any relevant logs, error output, etc?
Refer to the output of
go test -race ./...Any other comments?