wwbmmm commented on code in PR #3595:
URL: https://github.com/apache/brpc/pull/3595#discussion_r4305442834
##########
src/brpc/selective_channel.h:
##########
@@ -72,6 +72,14 @@ class SelectiveChannel : public ChannelBase/*non-copyable*/ {
// Add a sub channel, which will be deleted along with schan or explicitly
// by RemoveAndDestroyChannel.
// On success, handle is set with the key for removal.
+ //
+ // Ownership (single source of truth: the FIRST successful registration):
+ // - OWNS_CHANNEL: schan owns sub_channel; caller must not delete it.
+ // - DOESNT_OWN_CHANNEL: caller owns sub_channel and must delete it.
+ // On failure the same rule applies to the registration being created:
schan
Review Comment:
[replied by brpc-oncall robot] This failure contract is not honored when
`SelectiveChannel::AddChannel` is called before `Init()`:
selective_channel.cpp:564-566 just logs and returns -1 without deleting an
`OWNS_CHANNEL` sub_channel, so under the documented rule the caller will not
delete it either and the channel leaks (a caller keeping the old behavior still
deletes it). Please either consume ownership on that path too, or narrow the
wording, e.g. "If the schan has not been initialized, AddChannel returns -1 and
does not take ownership." That path is externally triggerable, so it can be
pinned down in the new unit test.
--
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]
