AlexStocks opened a new issue, #132:
URL: https://github.com/apache/dubbo-getty/issues/132
## Summary
I reviewed the current `master` branch of `apache/dubbo-getty` locally and
found several source-grounded lifecycle / concurrency / stability issues in the
transport layer that look worth fixing.
The findings below are intentionally limited to issues with concrete code
paths and clear impact, rather than broad speculation.
---
## 1. Client shutdown can deadlock because `client.stop()` closes sessions
while holding `c.Lock()`, and session close can synchronously re-enter client
reconnect logic
**Files**:
- `client.go`
- `session.go`
### Relevant code path
`client.stop()`:
```go
c.Lock()
for s := range c.ssMap {
s.RemoveAttribute(sessionClientKey)
s.RemoveAttribute(ignoreReconnectKey)
s.Close()
}
c.Unlock()
```
`session.stop()`:
```go
clt, cltFound := s.GetAttribute(sessionClientKey).(*client)
ignoreReconnect, flagFound := s.GetAttribute(ignoreReconnectKey).(bool)
if cltFound && flagFound && !ignoreReconnect {
clt.reConnect()
}
```
`reConnect()` calls methods that need the same client lock again (for
example through `sessionNum()` / `connect()`).
### Why this matters
This creates a shutdown-time lock re-entry path:
- client shutdown holds `c.Lock()`
- session close runs synchronously under that lock
- session close can call back into `clt.reConnect()`
- reconnect path needs the same client lock again
That is a real deadlock risk during client shutdown.
### Suggested fix
- never call `s.Close()` while holding the client map lock
- first snapshot sessions under lock, then release the lock, then close them
- more generally, avoid synchronous reconnect work inside the session
teardown path
---
## 2. Session teardown performs reconnect synchronously inside
`session.stop()`, which makes close/shutdown paths unexpectedly heavy and
fragile
**File**: `session.go`
### Relevant code path
```go
if cltFound && flagFound && !ignoreReconnect {
clt.reConnect()
}
```
`reConnect()` itself can:
- loop,
- dial,
- sleep/backoff,
- and inspect/update client session state.
### Why this matters
A session close path is expected to be teardown-only, but here it can
synchronously perform reconnection logic.
That means:
- `Close()` is no longer a small/local operation,
- teardown can block inside reconnect backoff/dial logic,
- and shutdown ordering becomes harder to reason about.
This also amplifies the deadlock problem above.
### Suggested fix
- decouple reconnect scheduling from `session.stop()`
- move reconnect decisions to a dedicated client lifecycle controller /
background worker
- make teardown publish state and return, rather than performing
reconnection inline
---
## 3. Deferred error path in `handlePackage()` can panic because of an
incorrect nil check
**File**: `session.go`
### Relevant code path
```go
if err != nil {
...
if s != nil || s.listener != nil {
s.listener.OnError(s, err)
}
}
```
The guard uses `||` instead of `&&`.
### Why this matters
If `s != nil` but `s.listener == nil`, the condition still passes and
`s.listener.OnError(...)` dereferences a nil listener.
That means a normal I/O / decode / shutdown error in the deferred cleanup
path can escalate into a panic.
### Suggested fix
- replace the guard with `if s != nil && s.listener != nil { ... }`
- add a regression test for the error-cleanup path with a nil listener
---
## 4. WebSocket self-connect rejection leaks the upgraded connection
**File**: `server.go`
### Relevant code path
```go
conn, err := s.upgrader.Upgrade(w, r, nil)
...
if conn.RemoteAddr().String() == conn.LocalAddr().String() {
log.Warnf(...)
return
}
```
After the websocket upgrade succeeds, the self-connect branch returns
directly without closing `conn`.
### Why this matters
At that point the connection has already been upgraded and allocated.
Returning without closing it leaks the socket/resource for that request path.
### Suggested fix
- explicitly close `conn` before returning in the self-connect rejection
branch
- add a test or at least a small harness covering this branch if possible
---
## 5. WSS event loop panics on normal server shutdown/error return paths
**File**: `server.go`
### Relevant code path
```go
err = server.Serve(tls.NewListener(s.streamListener, config))
if err != nil {
log.Errorf(...)
panic(err)
}
```
The non-TLS WS loop logs serve errors and returns, but the WSS loop panics
on any non-nil `Serve(...)` result.
### Why this matters
For long-running servers, `Serve(...)` returning is not always an
exceptional condition. During normal shutdown, listener closure often causes
`Serve` to return.
Treating that path as unconditional panic makes graceful shutdown brittle
and can convert expected shutdown into process crash.
### Suggested fix
- distinguish expected shutdown errors from unexpected serve failures
- handle normal listener/server close without panic
- keep panic only for genuinely impossible internal invariants, if any
---
## 6. UDP receive buffer sizing contains a dead branch / likely logic bug
**File**: `session.go`
### Relevant code path
```go
maxBufLen = int(s.maxMsgLen + maxReadBufLen)
if int(s.maxMsgLen<<1) < bufLen {
maxBufLen = int(s.maxMsgLen << 1)
}
```
At this point `bufLen` has not yet been populated from `conn.recv(buf)` and
is still zero.
### Why this matters
The condition is effectively dead on entry and does not do what it appears
to intend.
This looks like a real logic bug in UDP buffer sizing, and it likely means
the branch never contributes to runtime behavior.
### Suggested fix
- re-check the intended sizing rule
- if the intent was to cap by `2 * maxMsgLen`, compare against the right
value instead of uninitialized `bufLen`
- add a focused test around UDP buffer sizing / message length guard behavior
---
## 7. Reconnect attempt accounting appears monotonic across successes, which
may permanently disable reconnect after enough intermittent failures
**File**: `client.go`
### Relevant code path
```go
var reconnectAttempts int
...
for {
...
if connPoolSize <= sessionNum || maxReconnectAttempts <
reconnectAttempts {
break
}
c.connect()
reconnectAttempts++
...
}
```
`reconnectAttempts` increases each loop, but I did not see it reset after a
successful reconnect.
### Why this matters
If intermittent disconnect/reconnect cycles accumulate over process
lifetime, the client may eventually stop reconnecting entirely once it crosses
`maxReconnectAttempts`, even if previous reconnects actually succeeded.
### Suggested fix
- make retry accounting track consecutive reconnect failures rather than
total lifetime attempts in the loop
- reset the counter when pool health is restored / a reconnect succeeds
- add a regression test for repeated disconnect-success-disconnect cycles
---
## Notes
I did **not** try to turn every suspicious pattern into an issue here. The
items above are the ones that looked most actionable and had the clearest code
evidence.
--
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]
