AlexStocks opened a new issue, #989:
URL: https://github.com/apache/dubbo-go-pixiu/issues/989
## Summary
I reviewed the current `develop` branch of `apache/dubbo-go-pixiu` locally
and found several source-grounded stability / lifecycle issues that are worth
follow-up.
This issue is intentionally focused on problems that are either:
- obvious runtime bugs,
- goroutine / lifecycle / shutdown hazards,
- or panic-style failure handling that is too aggressive for ordinary
runtime/configuration errors.
---
## 1. `KafkaConsumerFacade.Subscribe()` can panic on first use because
`consumerManager` is never initialized
**File**: `pkg/client/mq/kafka_facade.go`
Relevant code path:
- `KafkaConsumerFacade` declares `consumerManager map[string]func()`
- `NewKafkaConsumerFacade(...)` initializes `consumerGroup`, `httpClient`,
`done`, but does **not** initialize `consumerManager`
- `Subscribe()` does:
```go
key := GetConsumerManagerKey(cOpt.TopicList, cOpt.ConsumerGroup)
f.consumerManager[key] = cancel
```
If `consumerManager` is still nil, that assignment panics immediately.
### Why this matters
This is not a theoretical issue: a nil map write in Go is a direct runtime
panic.
### Suggested fix
- initialize `consumerManager` in `NewKafkaConsumerFacade(...)`, e.g.
`make(map[string]func())`
- also review concurrent access to `consumerManager`, because it is later
read/written from background goroutines without synchronization
### Suggested follow-up test
- add a unit test that constructs a new consumer facade and calls
`Subscribe()` once
- expected behavior: no panic
---
## 2. Pixiu startup wait group appears permanently unbalanced, which can
block startup forever
**File**: `pkg/server/pixiu_start.go`
Relevant code path:
```go
func (s *Server) Start() {
...
s.startWG.Add(1)
...
}
func Start(bs *model.Bootstrap) {
...
server.Start()
server.startWG.Wait()
}
```
I did not find a matching `Done()` for this `startWG.Add(1)` in the startup
path.
### Why this matters
If the wait group counter is incremented and never decremented,
`server.startWG.Wait()` will block indefinitely.
This looks like a straightforward lifecycle bug unless there is an external
`Done()` path that I missed.
### Suggested fix
- either remove this wait group if it is no longer needed,
- or ensure the intended startup completion path calls `Done()` exactly once
- if the original intent was “block until shutdown”, the code should make
that explicit rather than using an apparently unbalanced startup wait group
### Suggested follow-up test
- add a focused startup lifecycle test that verifies `Start(bs)` does not
hang indefinitely under normal startup conditions
---
## 3. Several runtime/configuration paths still use `panic` / `os.Exit` /
fatal-style handling where normal error propagation would be safer
Representative files:
- `pkg/config/xds/apiclient/grpc_envoy.go`
- `pkg/server/listener_manager.go`
- `admin/core/viper.go`
- `admin/core/xds.go`
- `pkg/cmd/gateway.go`
- `pkg/filter/network/dubboproxy/plugin.go`
- `pkg/filter/network/dubboproxy/manager.go`
Examples:
- missing xDS cluster config leads to `panic(...)` in `grpc_envoy.go`
- graceful shutdown path in `listener_manager.go` uses `os.Exit(0)` both on
timeout and inside per-listener shutdown error handling
- config loading in multiple paths still uses direct panic instead of
returning structured startup errors
### Why this matters
These are not necessarily “security” bugs, but they are operational
stability issues:
- configuration mistakes or transient dependency failures can terminate the
whole process
- one listener shutdown failure can immediately exit the process while other
cleanup is still in progress
- panic/fatal behavior makes the system harder to embed, test, and recover
### Suggested fix
- audit panic/fatal usage and classify each call site into:
1. true invariant violation (panic may stay)
2. configuration/startup error (should return error)
3. background runtime failure (should be surfaced via lifecycle
management, not unconditional process exit)
### Suggested rollout strategy
This probably should **not** be one giant PR.
Better split by subsystem:
- xDS api client init path
- listener/shutdown path
- admin config loading path
- network filter/plugin construction path
---
## 4. Shutdown path in `ListenerManager` is fairly blunt and may skip
orderly cleanup on individual listener failure
**File**: `pkg/server/listener_manager.go`
Relevant code path:
- shutdown installs signal handling in `gracefulShutdownInit()`
- per listener, it starts a goroutine calling
`listener.ShutDown(lm.shutdownWG)`
- if any one returns error, it logs and immediately calls `os.Exit(0)`
inside that goroutine
- timeout path also calls `os.Exit(0)` via `time.AfterFunc`
### Why this matters
Even if the current behavior is “fail fast”, calling `os.Exit(0)` from a
worker goroutine during shutdown is extremely aggressive:
- remaining listeners may not finish their own cleanup
- metrics/tracing/log flush may not complete
- the process exits with success code even though shutdown encountered an
error
### Suggested fix
- collect shutdown errors instead of exiting from per-listener worker
goroutines
- wait for all listener shutdown attempts (or a coordinated timeout)
- return/report a consolidated shutdown result
- reserve force-exit for the outermost shutdown policy layer, not inside
individual worker goroutines
---
## 5. Potential concurrency follow-up: `KafkaConsumerFacade` uses shared
mutable state without synchronization
**File**: `pkg/client/mq/kafka_facade.go`
Beyond the nil-map panic above, `consumerManager` is also:
- written in `Subscribe()`
- read / invoked / deleted in `checkConsumerIsAlive()`
I did not fully prove a race with a reproducer here, but this map is clearly
shared across goroutines without a mutex or `sync.Map`.
### Why this matters
Once the nil-map issue is fixed, concurrent subscription / health-check
activity may still produce races or undefined behavior.
### Suggested fix
- protect `consumerManager` with a mutex, or replace it with `sync.Map`
- add a concurrent unit test or race-enabled test around subscribe +
health-check lifecycle
---
## Notes
I deliberately did **not** try to turn every `panic(...)` occurrence in the
repo into a bug here. Some are in tests, some are plugin registration
invariants, and some may be acceptable fail-fast behavior.
The items above are the ones that looked most actionable and most likely to
deserve community follow-up.
--
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]
