Skip to content

Commit 24e2523

Browse files
committed
Fix isClosing data race
This PR fixes a data race related to the webSocketClient isClosing field. Before this change the field was set in the Close method while holding a lock but read in the listenWebSocket method without holding a lock. The point of the isClosing flag was to prevent sending on errChan after close. Instead of having a hidden dependecy between a flag and the errChan channel state, this PR replaces isClosing with a flag named exitListenWebSocket, and when the flag is set the listenWebSocket closes errChan itself. listenWebSocket is the only goroutine that writes to errChan, so there's no longer any possibility of writing on a closed channel -- the listenWebSocket goroutine now effectively "owns" errChan.
1 parent 26f7f84 commit 24e2523

2 files changed

Lines changed: 23 additions & 23 deletions

File tree

Makefile

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ lint:
77
internal/lint/golangci-lint run ./... --fix
88

99
check: lint
10-
go test -cover ./...
10+
go test -race -cover ./...
1111
go mod tidy
1212

1313
.PHONY: example

graphql/websocket.go

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -44,15 +44,19 @@ const (
4444
)
4545

4646
type webSocketClient struct {
47-
Dialer Dialer
48-
header http.Header
49-
endpoint string
50-
conn WSConn
51-
connParams map[string]interface{}
47+
Dialer Dialer
48+
conn WSConn
49+
header http.Header
50+
connParams map[string]interface{}
51+
// Closed when exiting the receive loop in listenWebSocket
5252
errChan chan error
53+
endpoint string
5354
subscriptions subscriptionMap
54-
isClosing bool
55-
sync.Mutex
55+
56+
// Hold when accessing `exitListenWebSocket`
57+
exitListenWebSocketMu sync.Mutex
58+
// Set to indicate the listenWebSocket should exit
59+
exitListenWebSocket bool
5660
}
5761

5862
type webSocketInitMessage struct {
@@ -104,14 +108,6 @@ func (w *webSocketClient) waitForConnAck() error {
104108
return nil
105109
}
106110

107-
func (w *webSocketClient) handleErr(err error) {
108-
w.Lock()
109-
defer w.Unlock()
110-
if !w.isClosing {
111-
w.errChan <- err
112-
}
113-
}
114-
115111
func (w *webSocketClient) listenWebSocket() {
116112
for {
117113
// The listenWebSocket goroutine "owns" interfaceChan. Both sending
@@ -126,17 +122,20 @@ func (w *webSocketClient) listenWebSocket() {
126122
sub.interfaceChan = nil
127123
}
128124
})
129-
if w.isClosing {
125+
w.exitListenWebSocketMu.Lock()
126+
if w.exitListenWebSocket {
127+
close(w.errChan)
130128
return
131129
}
130+
w.exitListenWebSocketMu.Unlock()
132131
_, message, err := w.conn.ReadMessage()
133132
if err != nil {
134-
w.handleErr(err)
133+
w.errChan <- err
135134
return
136135
}
137136
err = w.forwardWebSocketData(message)
138137
if err != nil {
139-
w.handleErr(err)
138+
w.errChan <- err
140139
return
141140
}
142141
}
@@ -216,10 +215,11 @@ func (w *webSocketClient) Close() error {
216215
if err != nil {
217216
return fmt.Errorf("failed to send closure message: %w", err)
218217
}
219-
w.Lock()
220-
defer w.Unlock()
221-
w.isClosing = true
222-
close(w.errChan)
218+
219+
w.exitListenWebSocketMu.Lock()
220+
w.exitListenWebSocket = true
221+
w.exitListenWebSocketMu.Unlock()
222+
223223
return w.conn.Close()
224224
}
225225

0 commit comments

Comments
 (0)