lhotari commented on PR #1476:
URL:
https://github.com/apache/pulsar-client-go/pull/1476#issuecomment-4370610464
## Local review by Claude Code
This is a code review performed locally by Claude Code (model: Opus 4.7). No
automated tools posted this; a human reviewer ran `/pr-review` against the diff
and metadata.
**Verdict:** Approve. Bug is real, fix is correct, tests are thorough.
### Summary
The race is real and the fix is well-implemented. `GetConnection → _setConn
→ AddConsumeHandler → RequestOnCnx` mirrors the existing producer pattern in
`producer_partition.go:320-345`. Failure paths cleanly delete the handler,
restore the previous connection, and route the timeout `CLOSE_CONSUMER` over
the same `cnx` rather than re-resolving from the pool (which is actually
slightly more correct — consumer IDs are per-connection).
### Correctness checks confirmed
- `cnx` from `cnxPool.GetConnection` is identical to `res.Cnx` from
`RequestOnCnx(cnx, ...)` (`internal/rpc_client.go:175-186`), so removing the
post-success `_setConn(res.Cnx)` is safe.
- `pc.conn` is `atomic.Pointer[internal.Connection]` (not `atomic.Value`),
so `pc.conn.Store(nil)` does not panic.
- Reconnect callers discard the return value into a retry loop
(`consumer_partition.go:2055-2114`); the `prevConn` rollback preserves prior
semantics that `pc.conn` survives a failed reconnect unchanged.
### Nits (non-blocking)
1. **Triplicated cleanup block** — the "delete handler + restore prevConn
(or store nil)" sequence is inlined three times. A small `restoreConn := func()
{ ... }` closure or a `defer` gated on a `subscribed` flag would be cleaner.
2. **`_getConn()` invariant comment** at `consumer_partition.go:2496` claims
conn is non-nil for the lifetime of the consumer. In practice it's only non-nil
after the first successful subscribe (true before this PR too —
initial-subscribe failure left the zero-value pointer). Worth tightening the
comment in a follow-up.
3. **Test `connectedCh` drain** —
`TestGrabConn_HandlerRegisteredBeforeSubscribe` and
`TestGrabConn_BrokerFrameDuringSubscribe` hit the success path which spawns `go
func() { pc.connectedCh <- struct{}{} }()`. Channel is buffered (size 1) so the
goroutine completes, but draining it (`<-pc.connectedCh`) would assert
correctness and protect against future buffer-size changes.
4. **Helper init asymmetry** — `newGrabConnTestConsumerNoConn` initializes
`pc.availablePermits` but `newGrabConnTestConsumer` does not. A one-line
comment explaining that only the `MessageReceived` path needs it would help
future readers.
### Intent vs implementation
The PR description's claim that the fix "mirrors the existing pattern in
`producer_partition.go`" is accurate; the consumer version adds extra
`prevConn` rollback that the producer doesn't bother with, which is a small
improvement.
--
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]
