XnLemon commented on code in PR #3579:
URL: https://github.com/apache/dubbo-go/pull/3579#discussion_r3763636594
##########
protocol/jsonrpc/server.go:
##########
@@ -79,14 +79,20 @@ func NewServer() *Server {
}
func (s *Server) handlePkg(conn net.Conn) {
+ connectionCtx, connectionCancel :=
context.WithCancel(context.Background())
+ writeConn := &lockedConn{Conn: conn}
+ var requestWG sync.WaitGroup
defer func() {
if r := recover(); r != nil {
logger.Warnf("[Jsonrpc][Server] connection panic,
local=%v, remote=%v, err=%v, debug stack=%s",
conn.LocalAddr(), conn.RemoteAddr(), r,
string(debug.Stack()))
}
conn.Close()
+ requestWG.Wait()
}()
+ // Register this after the cleanup defer so LIFO ordering cancels
request contexts before Wait.
+ defer connectionCancel()
Review Comment:
Verified. The lockedConn wrapper only serializes individual writes; it does
not preserve request order, so a faster later invocation can write its response
before an earlier one and violate HTTP/1.1 pipelining semantics.
I will keep invocation execution asynchronous so the read loop can still
detect disconnects and cancel the connection context, but buffer each complete
response and serialize connection writes by request sequence. I will also add
the suggested regression test with request 1 blocked and request 2 completing
immediately, asserting that response 2 is not written first. I will leave this
thread open until the fix is pushed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
