two-headBoy commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4246997456
##########
src/brpc/socket.cpp:
##########
@@ -722,7 +722,10 @@ int Socket::Create(const SocketOptions& options, SocketId*
id) {
int Socket::OnCreated(const SocketOptions& options) {
if (_io_event.Init((void*)id()) != 0) {
LOG(ERROR) << "Fail to init IOEvent";
- SetFailed(ENOMEM, "%s", "Fail to init IOEvent");
+ // VersionedRefWithId::Create() performs the failure transition after
+ // OnCreated() returns an error; attach the user before returning so
+ // recycling invokes SocketUser::BeforeRecycle.
+ _user = options.user;
Review Comment:
Agreed, and this is the key insight that drove the fix. Attaching`_user`
here is wrong not just because of the two caller symptoms but
because`BeforeRecycled` would run on a half-constructed socket (`_transport`
not created,`_id_wait_list` not initialized). Rather than make`BeforeRecycled`
safe for partial construction (a separate, larger change), the`_user`
attachment is dropped from the failure path entirely — it is assigned only at
the end of`OnCreated` on success. That also resolves your "cannot be decided at
the call site" point: with`_user` attached only on success,`Socket::Create`
failure never triggers`BeforeRecycle` , so the contract becomes uniform —
callers always own cleanup when`Create` fails — and`AddChannel` can use a
single unconditional cleanup path with no ambiguity. (Making`BeforeRecycled`
itself robust to a partially-constructed socket is left out of scope as you
suggested.)
--
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]
