Copilot commented on code in PR #3099:
URL: https://github.com/apache/dubbo-go/pull/3099#discussion_r2588057209
##########
tools/dubbogo-cli/generator/sample/hessian/generator.go:
##########
@@ -210,17 +216,17 @@ func genRegistryPOJOStatements(file *fileInfo,
initFunctionStatement *[]byte) []
var rIndexList []int
for _, name := range hessianPOJOList {
statement := genRegistryPOJOStatement(name)
- if initFunctionStatement != nil { // 检测是否已存在注册方法体
+ if initFunctionStatement != nil { // Check whether a
registration statement already exists
r, _ = regexp.Compile(escape(string(statement)))
initStatement := *initFunctionStatement
rIndexList = r.FindIndex(initStatement)
if len(rIndexList) > 0 {
i := rIndexList[0]
n := lastIndexOf(initStatement, newLine, &i)
checkStatement :=
initStatement[lastIndexOf(initStatement, newLine, &n)+1 : i]
- if passed, _ := regexp.Match(LineCommentRegexp,
checkStatement); !passed { // 是否被行注释
+ if passed, _ := regexp.Match(LineCommentRegexp,
checkStatement); !passed { // Check if commented out
Review Comment:
This line still uses `regexp.Match(LineCommentRegexp, ...)` instead of the
pre-compiled `lineCommentRegexp` that was introduced at line 106. For
consistency with the optimization applied elsewhere in this function, this
should be changed to `lineCommentRegexp.Match(checkStatement)` to avoid
recompiling the regex on each iteration.
```suggestion
if !lineCommentRegexp.Match(checkStatement) {
// Check if commented out
```
##########
server/server.go:
##########
@@ -213,7 +216,15 @@ func (s *Server) Serve() error {
if err := exposed_tmp.RegisterServiceInstance(); err != nil {
return err
}
- select {}
+ // Listen for interrupt signals to enable graceful shutdown
+ // This replaces the previous select{} which blocked indefinitely and
couldn't respond to signals
+ sigChan := make(chan os.Signal, 1)
+ signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM, syscall.SIGINT)
+
+ // Wait for a signal to shutdown gracefully
+ sig := <-sigChan
+ logger.Infof("Received signal: %v, server is shutting down gracefully",
sig)
Review Comment:
The graceful shutdown implementation here conflicts with the existing
`graceful_shutdown` package. The `graceful_shutdown.Init()` function (called at
line 109 in server/options.go) already sets up signal handling when
`InternalSignal` is configured. This creates duplicate signal handlers and
doesn't properly invoke the existing graceful shutdown logic (e.g.,
`beforeShutdown()` which destroys registries, waits for active requests,
destroys protocols, and runs custom callbacks).
Instead of implementing signal handling here, you should either:
1. Remove this signal handling and rely on the existing `graceful_shutdown`
package's built-in signal handling, or
2. If manual shutdown is needed, call `graceful_shutdown.beforeShutdown()`
or similar cleanup functions before returning.
```suggestion
// Signal handling and graceful shutdown are managed by the
graceful_shutdown package.
```
##########
server/server.go:
##########
@@ -213,7 +216,15 @@ func (s *Server) Serve() error {
if err := exposed_tmp.RegisterServiceInstance(); err != nil {
return err
}
- select {}
+ // Listen for interrupt signals to enable graceful shutdown
+ // This replaces the previous select{} which blocked indefinitely and
couldn't respond to signals
Review Comment:
The comment states that `select{}` "couldn't respond to signals," but this
is misleading. In Go, `select{}` blocks indefinitely but can still be
interrupted by signal handlers registered with `signal.Notify()`. The actual
issue with `select{}` is that it blocks without any way to gracefully shut down
or clean up resources when the process receives a signal.
```suggestion
// This replaces the previous select{} which blocked indefinitely and
did not provide a way to gracefully shut down or clean up resources when the
process received a signal
```
--
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]
