Repository navigation
Fix wss event loop panics on normal server shutdown/error return paths - #138
Conversation
…-panics-on-normal-server-shutdown/error-return-paths
|
|
||
| func TestWSSServerCloseDoesNotPanic(t *testing.T) { | ||
| certFile := filepath.Join(t.TempDir(), "server.crt") | ||
| keyFile := filepath.Join(t.TempDir(), "server.key") |
There was a problem hiding this comment.
[P1] 补一条 unexpected Serve error 仍会失败的回归测试
当前用例只证明正常 Close() 不再触发 panic，没有保护本 PR 同时承诺保留的 fatal-error 路径。我在独立副本中把 isHTTPServeCloseError 变异为无条件返回 true 后，这个测试重复 20 次和全仓 go test ./... 仍全部通过，也就是所有 WSS Serve 故障被静默降级的回归无法被发现。请用可注入的 fake listener，必要时放到子进程中，分别验证 http.ErrServerClosed/wrapped net.ErrClosed 正常退出，而普通 fatal Accept/Serve error 必须进入 error 日志并保持 panic。
| func isHTTPServeCloseError(err error) bool { | ||
| return errors.Is(err, http.ErrServerClosed) || | ||
| errors.Is(err, net.ErrClosed) || | ||
| strings.Contains(err.Error(), "use of closed network connection") |
There was a problem hiding this comment.
[P2] 不要仅按错误文本判定正常关闭
这里的 substring 会把不属于关闭的真实故障一起吞掉。我用真实 http.Server.Serve 和故障 listener 返回 fatal listener failure after use of closed network connection marker，当前 helper 会判为正常关闭，WSS loop 因而跳过 logHTTPServeError 和 panic。仓库最低 Go 版本已能通过 errors.Is(err, net.ErrClosed) 识别标准 listener 关闭；建议移除文本匹配，或者至少与明确的 shutdown 状态/sentinel 链联合约束，并补对应分类测试。
|
@222twotwotwo good work. I have merged this PR. please continue your work if there are other problems. |
What this PR does:
This PR handles expected HTTP server shutdown errors in the WS/WSS event loops.
For WSS servers, normal shutdown or listener close errors are now treated as expected close paths instead of causing a panic. Unexpected WSS
Serveerrors still keep the original behavior: they are logged and then panic.It also adds a regression test to verify that closing a WSS server does not panic during normal shutdown.
Which issue(s) this PR fixes:
Fixes #132
Special notes for your reviewer:
The WSS panic behavior is preserved for unexpected
Servefailures. This PR only excludes expected shutdown errors such ashttp.ErrServerClosed,net.ErrClosed, and closed listener errors.Validated with:
go test ./...Does this PR introduce a user-facing change?: