Repository navigation
fix: serialize websocket write deadlines with message writes - #157
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
Add docstrings to the remaining WebSocket write-related helpers and the test helper/closure touched by the deadline fix, so the docstring coverage check sees the changed functions documented. No behavior change.
AlexStocks
left a comment
There was a problem hiding this comment.
工业级 review 结论:Approve。
验证范围:issue #153 缺陷④「并发 WS 发送 deadline 数据竞争」。
验证结果:
- 基线上
SetWriteDeadline在Send/writePing/writePong/CloseConn中各自调用,未被writeLock保护;-race检测到多个 goroutine 并发设置 deadline 的 DATA RACE。 - 本 PR 将
updateWriteDeadline移入threadSafeWriteMessage内部,在持有writeLock期间统一设置 deadline 并立即写入,消除竞态。 - 移除各调用点重复的
updateWriteDeadline调用,简化调用路径。 - 新增
TestWSConcurrentWriteDeadlines覆盖并发数据帧/ping/pong 写入及并发 close,-race下通过。
代码观察:
updateWriteDeadline现在要求调用方已持有writeLock,注释已明确说明,符合 gorilla/websocket 对单写者的要求。
未处理项:issue #153 缺陷⑥不在本 PR 范围内,需后续跟进。
What this PR does:
gettyWSConn.Send,writePing,writePongandCloseConneach calledupdateWriteDeadline()before enteringthreadSafeWriteMessage(). The write lock coveredWriteMessage, but not the deadline update.That is not safe under concurrency:
Session.SendtakespacketLock.RLock(), which is shared by design, so multipleSendcalls can entergettyWSConn.Sendat the same time.writePingdoes not takepacketLockat all,writePongruns on the read goroutine, andCloseConncan run from the session teardown goroutine.Conn.SetWriteDeadlineis justc.writeDeadline = t, and every frame flush reads that field. Two deadline updates are therefore a write-write race, and a deadline update concurrent with an in-flight write is a write-read race.go test -racereports it insidewebsocket.(*Conn).SetWriteDeadline.This PR makes the deadline update part of the same critical section as the frame write:
The four callers no longer update the deadline themselves. All WebSocket write paths —
Send, Ping, Pong and the close frame — now hold the same per-connectionwriteLockacross both the deadline update and the message write.updateWriteDeadlinealso snapshotswTimeoutonce instead of loading it twice. The guard and the value passed toSetWriteDeadlinecan no longer disagree ifSetWriteTimeoutruns concurrently. The write behavior itself is unchanged: each message still gets a fresh deadline, and a deadline error is still logged before the write is attempted.Which issue(s) this PR fixes:
Refs #153 — this PR only addresses item 4 (concurrent WebSocket sends racing on the write deadline). The other items in that issue are separate fixes.
Special notes for your reviewer:
writeLockis a field ongettyWSConn, so it is per connection: writers on one connection serialize, while different connections stay independent.w.conn.WriteMessageis only called fromthreadSafeWriteMessage;w.conn.SetWriteDeadlineis only called fromupdateWriteDeadline; andupdateWriteDeadlineis only called fromthreadSafeWriteMessage.CloseConnwrites the close frame under the lock. The subsequent transportCloseremains outside the lock; Gorilla permitsCloseconcurrently with other methods, and after the close frame is written Gorilla setswriteErrtoErrCloseSent, so waiting writers fail before sending a data frame. Theclose=truetest case covers this.SetCompressTypealready uses the samewriteLockto keep compression configuration from racing with writers; this change is consistent with that.CloseConn(-1)instead ofCloseConn(0).CloseConnforwards its argument toSetLinger;0enablesSO_LINGERwith a zero timeout, which sends an RST and can discard the just-written close frame, making the peer observe 1006 instead of a clean 1000 close.-1restores the OS-default close. This is test-only; the library'sCloseConnbehavior is unchanged.Testing:
TestWSConcurrentWriteDeadlinesintransport/connection_ws_test.goruns twoSession.Sendgoroutines pluswritePingandwritePong, each for 100 iterations, with an optional concurrentCloseConn.close=falseandclose=truemodes. It validates the peer's received messages and control frames, so it checks correctness as well as the absence of the data race.connection.go, the test fails withWARNING: DATA RACEbetween twoSetWriteDeadlinecalls. With the fix it passes, including-race -count=100, which was run to stress the close path.go test ./transport -count=1andgo test -race ./transport -count=1pass locally, as dogofmt,go vetandgit diff --check.Does this PR introduce a user-facing change?:
Summary by CodeRabbit