Copilot commented on code in PR #993:
URL: https://github.com/apache/dubbo-go-pixiu/pull/993#discussion_r3479864807
##########
pkg/server/listener_manager.go:
##########
@@ -98,29 +98,61 @@ func (lm *ListenerManager) gracefulShutdownInit() {
sig := <-signals
logger.Infof("get signal %s, dubbo-go-pixiu will start
shutdown.", sig)
- time.AfterFunc(timeout, func() {
- logger.Warn("Shutdown gracefully timeout, listeners
will shutdown immediately. ")
- os.Exit(0)
- })
+ // Create error collection channel with capacity for all
listeners
+ errCh := make(chan error, len(lm.activeListenerService))
+ // Start shutdown for all listeners
for _, listener := range lm.activeListenerService {
lm.shutdownWG.Add(1)
go func(listener *wrapListenerService) {
+ // Note: listener.ShutDown() internally calls
wg.Done()
err := listener.ShutDown(lm.shutdownWG)
if err != nil {
logger.Errorf("Shutdown Error: %+v",
err)
- os.Exit(0)
+ errCh <- err
}
}(listener)
}
- lm.shutdownWG.Wait()
+
+ // Wait for all shutdowns to complete or timeout
+ done := make(chan struct{})
+ go func() {
+ lm.shutdownWG.Wait()
+ close(done)
+ }()
Review Comment:
`done` is currently closed based on `lm.shutdownWG.Wait()`, but
`lm.shutdownWG.Done()` is invoked inside `listener.ShutDown()` before the
worker goroutine logs/sends the error into `errCh`. That means the WaitGroup
can reach zero and the code can select `<-done` and drain `errCh` before late
errors are sent, causing missed shutdown errors and an incorrect exit code (0)
even when a listener returned an error.
##########
pkg/server/listener_manager_test.go:
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package server
+
+import (
+ "sync"
+ "testing"
+ "time"
+)
+
+// TestShutdownWaitGroupDoubleDoneWouldPanic demonstrates that calling Done()
+// twice on a WaitGroup with counter 1 would panic (negative counter).
+func TestShutdownWaitGroupDoubleDoneWouldPanic(t *testing.T) {
+ wg := &sync.WaitGroup{}
+ wg.Add(1)
+
+ // First Done() - should be fine
+ wg.Done()
+
+ // Second Done() - should panic because counter becomes negative
+ defer func() {
+ if r := recover(); r == nil {
+ t.Error("Expected panic when Done() called twice on
WaitGroup with counter 1")
+ }
+ }()
+
+ wg.Done() // This should panic
+}
+
+// TestShutdownErrorCollection verifies that shutdown errors are collected
+// instead of causing immediate exit.
+func TestShutdownErrorCollection(t *testing.T) {
+ numListeners := 3
+ errCh := make(chan error, numListeners)
Review Comment:
These tests don’t exercise `ListenerManager.gracefulShutdownInit()` (signal
handling, WaitGroup coordination, error aggregation, exit-code selection). As a
result, the suite can still pass even if the production shutdown behavior
regresses. Consider extracting the shutdown coordination into a helper that
returns `(timedOut bool, errs []error)` (or similar) without calling `os.Exit`,
and unit-test that helper with fake listeners.
--
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]
