You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "Aias00 (via GitHub)" <gi...@apache.org> on 2026/06/26 09:32:13 UTC
[PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Aias00 opened a new pull request, #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995
## Summary
Fixes #989 Issue 3 (admin config loading path): Replace panic/os.Exit with proper error handling in admin config loading.
### Problem
The original implementation used panic and os.Exit in multiple places:
- `viper.go`: panic on config file read error
- `xds.go`: os.Exit(1) on startup validation failures
- `xds.go`: panic/os.Exit in background goroutine on runtime failures
### Changes in viper.go
- `Viper()` now returns `(*viper.Viper, error)` instead of panic on config read error
- Update `server.go` caller to handle the error gracefully
### Changes in xds.go
- **Startup errors**: return error instead of os.Exit(1)
- config.Consistent() failure → return error
- snapshot.SetSnapshot() failure → return error
- **Background goroutine**: log and continue instead of exit
- WatchWithPrefix error → log and return (keep running)
- config reload errors → log and continue (keep previous valid config)
- **Invariant violation**: keep panic in makeHTTPFilter (true programming error)
### Classification
| Location | Type | Classification | Fix |
|----------|------|----------------|-----|
| viper.go:59 | panic | Config/startup error | Return error |
| xds.go:95,101 | os.Exit | Config/startup error | Return error |
| xds.go:151,160,166 | panic/exit | Runtime failure | Log and continue |
| xds.go:207 | panic | True invariant | Keep |
### Why This Matters
Background goroutines should not exit the process directly. They should:
- Log errors and continue running with previous valid state
- Optionally signal for graceful shutdown
## Test Plan
- Build passes: `go build ./admin/...`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-4808594626
## [Codecov](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `13 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 26.30%. Comparing base ([`c3df76a`](https://app.codecov.io/gh/apache/dubbo-go-pixiu/commit/c3df76a0fc1652a1dc9da9e910bf83ec91369ed5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4c7f1ef`](https://app.codecov.io/gh/apache/dubbo-go-pixiu/commit/4c7f1eff80720782261929591a253a03014223f2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [admin/core/server.go](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995?src=pr&el=tree&filepath=admin%2Fcore%2Fserver.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YWRtaW4vY29yZS9zZXJ2ZXIuZ28=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [admin/core/xds.go](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995?src=pr&el=tree&filepath=admin%2Fcore%2Fxds.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YWRtaW4vY29yZS94ZHMuZ28=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [admin/core/viper.go](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995?src=pr&el=tree&filepath=admin%2Fcore%2Fviper.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YWRtaW4vY29yZS92aXBlci5nbw==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## develop #995 +/- ##
========================================
Coverage 26.29% 26.30%
========================================
Files 275 275
Lines 21985 21989 +4
========================================
+ Hits 5782 5785 +3
- Misses 15590 15592 +2
+ Partials 613 612 -1
```
| [Flag](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [unittests](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.30% <0.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/dubbo-go-pixiu/pull/995?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-4950185959
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks commented on code in PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#discussion_r3563657984
##########
admin/core/server.go:
##########
@@ -48,9 +48,13 @@ type server interface {
}
// RunServer start server
-func RunServer() {
+func RunServer() error {
// load config
- global.VP = Viper()
+ vp, err := Viper()
Review Comment:
[P1] CLI 的 `--config/-c` 写入 `cmd/admin.configPath`，但新的错误传播链仍然调用无参数 `Viper()`，没有把该路径传入；`Viper()` 会继续走自己的默认/stdlib flag 逻辑。结果是用户显式指定的 Admin 配置文件仍可能被忽略。请把 `configPath` 通过 `Start(configPath) → RunServer(configPath) → Viper(configPath)` 明确传递，并增加非默认路径测试。
##########
admin/core/server.go:
##########
@@ -83,4 +87,5 @@ func RunServer() {
})
wg.Wait()
+ return nil
Review Comment:
[P0] 新增的 `error` 返回实际上无法传播 HTTP/xDS 启动失败：两个 worker 在 `ListenAndServe` 或 `StartxDsServer` 出错时都只记录日志并正常返回，`wg.Wait()` 结束后这里固定返回 `nil`。因此端口占用、xDS snapshot 初始化失败等场景下，`cmd/admin` 的 `errCh` 仍会报告成功或继续等待，进程形成假健康状态。worker 必须把首个非预期错误发送到协调通道并触发其余服务关闭，`RunServer` 再将该错误返回 CLI；同时补充端口绑定失败和 xDS 初始化失败测试。
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-4951500722
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-5970864588
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on code in PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#discussion_r3565778773
##########
admin/core/server.go:
##########
@@ -48,9 +48,13 @@ type server interface {
}
// RunServer start server
-func RunServer() {
+func RunServer() error {
// load config
- global.VP = Viper()
+ vp, err := Viper()
Review Comment:
Fixed in `e6a14199`.
`configPath` is now threaded explicitly through the whole chain: `Start(configPath)` → `RunServer(configPath)` → `Viper(configPath)`. The CLI's `--config/-c` value (already held in `cmd/admin.configPath` and defaulted in `initDefaultValue()`) is passed into `Start(configPath)` in `RunE`, so a user-specified admin config file is loaded instead of being silently ignored.
In `Viper` I also removed the stdlib `flag.StringVar`/`flag.Parse()` fallback (it conflicted with cobra's flag ownership and was the root cause of the path being ignored). `Viper(configPath)` now resolves the path purely from the argument, falling back to the `GVA_CONFIG` env var and then the default `config.yaml` only when `configPath` is empty.
Tests added in `admin/core/server_test.go`:
- `TestViper_NonDefaultPathHonored` — writes a temp `config.yaml` with `system.addr: 9999`, calls `Viper(tmpPath)`, asserts `global.CONFIG.System.Addr == 9999` and that `vp.ConfigFileUsed()` is the explicit path (not the default).
- `TestViper_MissingFileReturnsError` — a non-existent explicit path returns a non-nil error (not silently defaulted).
##########
admin/core/server.go:
##########
@@ -83,4 +87,5 @@ func RunServer() {
})
wg.Wait()
+ return nil
Review Comment:
Fixed in `e6a14199`.
Worker startup errors now propagate to the CLI. The HTTP server and xDS server run under a new `runCoordinated(ctx, ...workers)` harness: the first worker to return a non-nil error cancels its peer and that error is returned by `RunServer`. Concretely:
- `runHTTPServer` returns `ListenAndServe` failures (e.g. port already in use), wrapped as `"http server: %w"`, instead of only logging.
- `StartxDsServer(ctx)` / `runXDSServer(ctx, ...)` take the context; the initial `net.Listen` error (port-bind failure) is returned, and on peer-cancel the gRPC server does `GracefulStop()` with the `Serve` error drained.
- The HTTP peer shuts down via `http.Server.Shutdown(ctx)` when the xDS server fails (first real `ErrServerClosed` trigger in the package), so neither worker lingers and the process exits non-zero instead of hanging on `wg.Wait()`.
Tests added in `admin/core/server_test.go`: `TestRunCoordinated_FirstErrorWins`, `TestRunCoordinated_OnlyFirstErrorPropagated`, `TestRunCoordinated_NoErrorReturnsNil`, `TestRunCoordinated_ParentCancelPropagates`, and `TestRunXDSServer_PortBindFailure` (occupies a port on all interfaces, asserts `runXDSServer` returns the bind error rather than hanging).
Side effect: drops the `v.marlon.life/toolkit/util` (`WaitGroupWrapper`) fork dependency from `server.go` in favor of `sync.WaitGroup` + `context`, aligning with develop (PR #984).
`go build`, `go vet`, `go test -race`, `gofmt`, and `golangci-lint` are all clean on `admin/core` and `cmd/admin`.
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on code in PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#discussion_r3565778342
##########
admin/core/server.go:
##########
@@ -83,4 +87,5 @@ func RunServer() {
})
wg.Wait()
+ return nil
Review Comment:
Fixed in `e6a14199`.
Worker startup errors now propagate to the CLI. The HTTP server and xDS server run under a new `runCoordinated(ctx, ...workers)` harness: the first worker to return a non-nil error cancels its peer and that error is returned by `RunServer`. Concretely:
- `runHTTPServer` returns `ListenAndServe` failures (e.g. port already in use), wrapped as `"http server: %w"`, instead of only logging.
- `StartxDsServer(ctx)` / `runXDSServer(ctx, ...)` take the context; the initial `net.Listen` error (port-bind failure) is returned, and on peer-cancel the gRPC server does `GracefulStop()` with the `Serve` error drained.
- The HTTP peer shuts down via `http.Server.Shutdown(ctx)` when the xDS server fails (first real `ErrServerClosed` trigger in the package), so neither worker lingers and the process exits non-zero instead of hanging on `wg.Wait()`.
Tests added in `admin/core/server_test.go`: `TestRunCoordinated_FirstErrorWins`, `TestRunCoordinated_OnlyFirstErrorPropagated`, `TestRunCoordinated_NoErrorReturnsNil`, `TestRunCoordinated_ParentCancelPropagates`, and `TestRunXDSServer_PortBindFailure` (occupies a port on all interfaces, asserts `runXDSServer` returns the bind error rather than hanging).
Side effect: drops the `v.marlon.life/toolkit/util` (`WaitGroupWrapper`) fork dependency from `server.go` in favor of `sync.WaitGroup` + `context`, aligning with develop (PR #984).
`go build`, `go vet`, `go test -race`, `gofmt`, and `golangci-lint` are all clean on `admin/core` and `cmd/admin`.
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-4815686715
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-4950231113
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-4808948404
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on code in PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#discussion_r3565778773
##########
admin/core/server.go:
##########
@@ -48,9 +48,13 @@ type server interface {
}
// RunServer start server
-func RunServer() {
+func RunServer() error {
// load config
- global.VP = Viper()
+ vp, err := Viper()
Review Comment:
Fixed in `e6a14199`.
`configPath` is now threaded explicitly through the whole chain: `Start(configPath)` → `RunServer(configPath)` → `Viper(configPath)`. The CLI's `--config/-c` value (already held in `cmd/admin.configPath` and defaulted in `initDefaultValue()`) is passed into `Start(configPath)` in `RunE`, so a user-specified admin config file is loaded instead of being silently ignored.
In `Viper` I also removed the stdlib `flag.StringVar`/`flag.Parse()` fallback (it conflicted with cobra's flag ownership and was the root cause of the path being ignored). `Viper(configPath)` now resolves the path purely from the argument, falling back to the `GVA_CONFIG` env var and then the default `config.yaml` only when `configPath` is empty.
Tests added in `admin/core/server_test.go`:
- `TestViper_NonDefaultPathHonored` — writes a temp `config.yaml` with `system.addr: 9999`, calls `Viper(tmpPath)`, asserts `global.CONFIG.System.Addr == 9999` and that `vp.ConfigFileUsed()` is the explicit path (not the default).
- `TestViper_MissingFileReturnsError` — a non-existent explicit path returns a non-nil error (not silently defaulted).
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-6208097858
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-4808306010
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-4808760243
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#issuecomment-4810240061
## [](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_dubbo-go-pixiu&pullRequest=995&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_dubbo-go-pixiu&pullRequest=995&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_dubbo-go-pixiu&pullRequest=995)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#discussion_r3480483135
##########
admin/core/xds.go:
##########
@@ -92,13 +91,13 @@ func StartxDsServer() error {
config := GenerateSnapshotPixiu()
if err := config.Consistent(); err != nil {
logger.Errorf("config inconsistency: %+v\n%+v", config, err)
- os.Exit(1)
+ return fmt.Errorf("config inconsistency: %w", err)
}
Review Comment:
StartxDsServer() logs the full inconsistency (including the snapshot contents) and also returns an error; the caller (admin/core/server.go) logs the returned error again. This can lead to duplicate log lines and potentially very noisy logs. Consider either removing this log here (and rely on the returned error) or returning richer context and letting the top-level decide what/how to log.
##########
admin/core/server.go:
##########
@@ -50,7 +50,12 @@ type server interface {
// RunServer start server
func RunServer() {
// load config
- global.VP = Viper()
+ vp, err := Viper()
+ if err != nil {
+ fmt.Printf("load config error: %v\n", err)
+ return
+ }
Review Comment:
On config load failure, RunServer() now just prints and returns. The admin CLI (cmd/admin/admin.go) calls core.RunServer() and then blocks waiting for a shutdown signal, so a config error can leave the process running without starting servers (and likely exiting with code 0). Consider changing RunServer() to return an error and have the Cobra command fail fast (e.g., switch to RunE and return the error) so startup failure is surfaced to callers/automation.
##########
admin/core/xds.go:
##########
@@ -148,7 +147,9 @@ func watchConfigAndReload() {
if err != nil {
logger.Errorf("watch config error %q", err)
- panic(err)
+ // Log the error and return - don't panic in background goroutine
+ // The server should continue running with existing config
+ return
Review Comment:
If WatchWithPrefix fails, watchConfigAndReload() now returns and permanently stops config reloads. For transient etcd/network errors this means the xDS server will never pick up future config changes. Consider retrying the watch with a backoff (and also restarting the watch if the channel closes) so reload functionality can recover.
##########
admin/core/viper.go:
##########
@@ -56,7 +56,7 @@ func Viper(path ...string) *viper.Viper {
v.SetConfigType("yaml")
err := v.ReadInConfig()
if err != nil {
- panic(fmt.Errorf("fatal error config file: %s", err))
+ return nil, fmt.Errorf("fatal error config file: %s", err)
Review Comment:
Use %w when returning the config read error so callers can unwrap/inspect it with errors.Is / errors.As. Using %s stringifies the underlying error and loses the original cause.
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "AlexStocks (via GitHub)" <gi...@apache.org>.
AlexStocks commented on code in PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#discussion_r4180466818
##########
admin/core/xds.go:
##########
@@ -132,12 +132,20 @@ func runXDSServer(srv envoyServer.Server, lis net.Listener) error {
grpcServer := grpc.NewServer(grpcOptions...)
registerServer(grpcServer, srv)
-
logger.Infof("management server listening on %s", lis.Addr())
- if err := grpcServer.Serve(lis); err != nil {
+
+ serveErr := make(chan error, 1)
+ go func() {
+ serveErr <- grpcServer.Serve(lis)
+ }()
+
+ select {
+ case err := <-serveErr:
return err
+ case <-ctx.Done():
+ grpcServer.GracefulStop()
+ return <-serveErr
Review Comment:
[P2] `GracefulStop()` 之后的返回值被当成了失败，会误报 xDS 监听错误。
`ctx.Done()` 分支是 `grpcServer.GracefulStop(); return <-serveErr`。我用仓库内实际的 gRPC 版本实测了这个返回值（存证：`evidence/probe-gracefulstop.txt`）：
```
Serve returned grpc: the server has been stopped after GracefulStop
```
也就是 `Serve` 在优雅停止后返回的是 `grpc.ErrServerStopped`，不是 `nil`。这个值经 `return <-serveErr` 一路传出 `runXDSServer` → `StartxDsServer`，然后命中 `admin/core/xds.go:107-110`：
```go
if err := runXDSServer(ctx, srv, lis); err != nil {
adminxds.DefaultStatusStore.RecordListenError(err) // <- 记成"xDS 没在监听"
return err
}
```
`RecordListenError` 会写 `status.Listening = false` 和 `status.ListenError`（`admin/xds/status.go:111-113`），而这个状态是**对外可观察**的：`admin/controller/xds/status.go:71` 用 `status.ListenError != ""` 判定 `Degraded`。也就是说，一次**完全正常**的优雅关闭（HTTP 侧先失败或收到信号 → cancel → xDS 优雅停止）会让 xDS 状态接口在退出瞬间报"降级 + 监听失败"。
**关于退出码，这条我核实后认为不是问题**：`runCoordinated` 的 `errCh` 容量为 1 且用 `default` 丢弃后来者，所以先到的真实错误（`http server: ...` 或 `net.Listen` 的错误）一定胜出，`ErrServerStopped` 抢不到。`RunServer` 的返回值语义是对的，这里不需要改。
建议在 `runXDSServer` 里把优雅停止和真实失败区分开：
```go
case <-ctx.Done():
grpcServer.GracefulStop()
if err := <-serveErr; err != nil && !errors.Is(err, grpc.ErrServerStopped) {
return err
}
return nil
```
这样 `RecordListenError` 就只会记录真正的监听失败。
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix(admin): replace panic/os.Exit with error handling in config loading [dubbo-go-pixiu]
Posted by "Aias00 (via GitHub)" <gi...@apache.org>.
Aias00 commented on code in PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#discussion_r3565777751
##########
admin/core/server.go:
##########
@@ -83,4 +87,5 @@ func RunServer() {
})
wg.Wait()
+ return nil
Review Comment:
test reply (will delete)
##########
admin/core/server.go:
##########
@@ -83,4 +87,5 @@ func RunServer() {
})
wg.Wait()
+ return nil
Review Comment:
test reply (will delete)
--
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: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org