rajvarun77 commented on PR #3330: URL: https://github.com/apache/brpc/pull/3330#issuecomment-4710940758
@wwbmmm @yanglimingcn @chenBright — this is green on all lanes and ready for another look. The earlier `brpc_channel_unittest` failure that I'd thought was an unrelated flake turned out to be a real bug this PR introduced: the new `bind_sock_action` member of `Controller::Call` was not default-initialized, so the backup/retry call (built via the `Call` copy constructor) read an indeterminate value in `Call::OnComplete`, which branches on it before the normal pool-return / `SetFailed` path. When the garbage happened to match `BIND_SOCK_RESERVE`/`BIND_SOCK_USE`, the backup call's socket was diverted instead of returned, leaving the in-flight RPC unconcluded and hanging `ChannelTest.backup_request` (nondeterministically, hence the flaky appearance). Fixed by giving the member an in-class default initializer (`BIND_SOCK_NONE`) so every `Call` construction path is safe; a backup/retry never inherits transaction affinity. `ChannelTest.backup_request` now passes in under a second. All prior review comments are addressed. The one remaining open thread — whether per-`Socket` `_fd_version` needs to be atomic — has a reply explaining why: it is written on the reconnect path with no socket lock held and read from another thread, so the relaxed atomic is there to make that cross-thread access race-free, independent of the global counter's uniqueness. Happy to adjust if you'd prefer a different framing. Could you take another pass when you have a moment? -- 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]
