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.\n\nI 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]
