Skip to content

http: prefer to return reference rather than shared ptr for clusterInfo()#44068

Merged
wbpcode merged 7 commits into
envoyproxy:mainfrom
wbpcode:dev-cluster-info-new-style
Mar 25, 2026
Merged

http: prefer to return reference rather than shared ptr for clusterInfo()#44068
wbpcode merged 7 commits into
envoyproxy:mainfrom
wbpcode:dev-cluster-info-new-style

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Mar 21, 2026

Commit Message: http: prefer to return reference rather than shared ptr for clusterInfo()
Additional Description:

See #44025 for more detail.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

…fo()

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
wbpcode added 2 commits March 22, 2026 11:29
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Comment on lines 334 to 336
Upstream::ClusterInfoConstSharedPtr cluster() override {
return decoder_filter_callbacks_.clusterInfo();
return decoder_filter_callbacks_.clusterInfoSharedPtr();
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this one be split into OptRef and SharedPtr variants too?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will prefer to make this be another PR because we need evaluate the usage of the cluster() before changes.

EXPECT_EQ("", route_config.name());
EXPECT_EQ(0, route_config.internalOnlyHeaders().size());
auto cluster_info = filter_callbacks->clusterInfo();
auto cluster_info = filter_callbacks->clusterInfoSharedPtr();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this one be using the optref variant since it's function-scoped?

Comment thread source/common/tcp_proxy/tcp_proxy.h Outdated
Comment thread test/common/http/conn_manager_impl_test.cc Outdated
Comment thread test/common/http/conn_manager_impl_test_2.cc Outdated
Comment thread test/extensions/filters/http/ext_authz/ext_authz_test.cc Outdated
Comment thread test/extensions/filters/http/grpc_web/grpc_web_filter_test.cc Outdated
wbpcode added 2 commits March 24, 2026 02:53
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Mar 24, 2026

/retest

Comment thread source/common/tcp_proxy/tcp_proxy.h Outdated
Comment thread test/mocks/http/mocks.cc
Comment on lines +63 to +69
.WillByDefault(Invoke([&callbacks]() -> OptRef<const Upstream::ClusterInfo> {
return makeOptRefFromPtr<const Upstream::ClusterInfo>(callbacks.cluster_info_.get());
}));
ON_CALL(callbacks, clusterInfoSharedPtr())
.WillByDefault(Invoke([&callbacks]() -> Upstream::ClusterInfoConstSharedPtr {
return callbacks.cluster_info_;
}));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.WillByDefault(Invoke([&callbacks]() -> OptRef<const Upstream::ClusterInfo> {
return makeOptRefFromPtr<const Upstream::ClusterInfo>(callbacks.cluster_info_.get());
}));
ON_CALL(callbacks, clusterInfoSharedPtr())
.WillByDefault(Invoke([&callbacks]() -> Upstream::ClusterInfoConstSharedPtr {
return callbacks.cluster_info_;
}));
.WillByDefault([&callbacks]() -> OptRef<const Upstream::ClusterInfo> {
return makeOptRefFromPtr<const Upstream::ClusterInfo>(callbacks.cluster_info_.get());
});
ON_CALL(callbacks, clusterInfoSharedPtr())
.WillByDefault([&callbacks]() -> Upstream::ClusterInfoConstSharedPtr {
return callbacks.cluster_info_;
});

(Invoke is redundant in modern gmock.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although we can skip the Invoke, but because Invoke be used every where in the test, it may make sense to keep consistency?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how you end up with keeping on replicating outdated code style forever. :)

Not too bothered in this PR though if you want to leave it. Maybe I should have an AI go through and remove all the mock Invokes repo-wide so we can stop cargo-cult-duplicating more of them in. :)

wbpcode added 2 commits March 24, 2026 16:57
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Router::RouteConstSharedPtr route() override { return route_; }
OptRef<const Upstream::ClusterInfo> clusterInfo() override {
return makeOptRefFromPtr<const Upstream::ClusterInfo>(clusterInfoSharedPtr().get());
const auto info =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will still require a change if info gets updated to returning a ref (because this will try to make a copy), so it might be better to just put it all in one very long line for future compatibility, but at least this way it'll be a compile-time error rather than a thing that can be accidentally left "the wrong way". :)

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Mar 25, 2026

/retest

@wbpcode wbpcode merged commit 4227d40 into envoyproxy:main Mar 25, 2026
29 checks passed
@wbpcode wbpcode deleted the dev-cluster-info-new-style branch March 25, 2026 11:48
fishcakez pushed a commit to fishcakez/envoy that referenced this pull request Mar 25, 2026
…fo() (envoyproxy#44068)

Commit Message: http: prefer to return reference rather than shared ptr
for clusterInfo()
Additional Description:

See envoyproxy#44025 for more detail.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

---------

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
TAOXUY pushed a commit to TAOXUY/envoy that referenced this pull request Apr 1, 2026
…fo() (envoyproxy#44068)

Commit Message: http: prefer to return reference rather than shared ptr
for clusterInfo()
Additional Description:

See envoyproxy#44025 for more detail.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

---------

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: Xuyang Tao <taoxuy@google.com>
citrus7 pushed a commit to citrus7/envoy that referenced this pull request Apr 1, 2026
…fo() (envoyproxy#44068)

Commit Message: http: prefer to return reference rather than shared ptr
for clusterInfo()
Additional Description:

See envoyproxy#44025 for more detail.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

---------

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: Jonathan Wu <jtwu@google.com>
nshipilov pushed a commit to nshipilov/envoy that referenced this pull request Apr 13, 2026
…fo() (envoyproxy#44068)

Commit Message: http: prefer to return reference rather than shared ptr
for clusterInfo()
Additional Description:

See envoyproxy#44025 for more detail.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

---------

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>
krinkinmu pushed a commit to grnmeira/envoy that referenced this pull request Apr 20, 2026
…fo() (envoyproxy#44068)

Commit Message: http: prefer to return reference rather than shared ptr
for clusterInfo()
Additional Description:

See envoyproxy#44025 for more detail.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

---------

Signed-off-by: wbpcode/wangbaiping <wbphub@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants