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. 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]
