balancer: Enforce embedding the SubConn interface in implementations #7758

arjan-bal · 2024-10-18T10:44:50Z

Presently, the doc comment on the balancer.SubConn interface mentions that external implementers should embed the balancer.SubConn interface, however, this is not enforced at compilation time:

// This interface is to be implemented by gRPC. Users should not need their own
// implementation of this interface. For situations like testing, any
// implementations should embed this interface. This allows gRPC to add new
// methods to this interface.

This PR adds an unexported method on the interface to add a compile time check for enforcing the requirement.

Release notes transferred to new PR that reworks this slightly.

RELEASE NOTES: None

codecov · 2024-10-18T10:49:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.81%. Comparing base (d66fc3a) to head (152cbce).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7758      +/-   ##
==========================================
+ Coverage   81.71%   81.81%   +0.09%     
==========================================
  Files         374      371       -3     
  Lines       38166    37700     -466     
==========================================
- Hits        31188    30844     -344     
+ Misses       5699     5563     -136     
- Partials     1279     1293      +14

Files with missing lines	Coverage Δ
balancer/balancer.go	`96.77% <ø> (ø)`
balancer_wrapper.go	`84.03% <ø> (ø)`
internal/testutils/balancer.go	`84.32% <ø> (ø)`

... and 33 files with indirect coverage changes

dfawley

This is a potential breaking change. I suspect it would only break tests (doing mocking?), but it's possible to break actual users. That's OK, we just need to be careful to document the change and also take care when importing the code internally.

dfawley · 2024-10-18T15:53:50Z

balancer/balancer.go

@@ -155,6 +155,9 @@ type SubConn interface {
 	// indicate the shutdown operation.  This may be delivered before
 	// in-progress RPCs are complete and the actual connection is closed.
 	Shutdown()
+	// enforceEmbedding is an unexported method to enforce implementers embed


"...to force implementers to embed..." or "to enforce that implementers embed".

Grammar fixed.

arjan-bal · 2024-10-21T07:19:14Z

This is a potential breaking change. I suspect it would only break tests (doing mocking?), but it's possible to break actual users. That's OK, we just need to be careful to document the change and also take care when importing the code internally.

@dfawley , I checked the internal usages and found that adding a private method will break code generated using GoMock, e.g: https://github.com/GoogleCloudPlatform/grpc-gcp-go/blob/d2bc599b72aeef2d85143b234ca7d602ae7f4302/grpcgcp/mocks/mock_balancer.go#L116-L119

Due to the private method, the generated mock no longer implements the balancer.SubConn interface, even though the mock has a private method with the same signature.

Users could manually edit the generated mocks to embed balancer.SubConn. Do you think it's okay to expect users manually edit their mocks?

easwars · 2024-10-21T19:44:14Z

I checked the internal usages and found that adding a private method will break code generated using GoMock

Hmm.. at least internally, mocking is not recommended for Go. I'm not sure about the external world though. Do you have any experience using mocking frameworks and know what is more commonly used and whether they generate code differently wherein the generated code embeds the mocked interface? (some google searching didn't yield anything useful)

Users could manually edit the generated mocks to embed balancer.SubConn. Do you think it's okay to expect users manually edit their mocks?

I guess this is no worse if you didn't use mocking and didn't embed, but chose to implement the methods to satisfy the interface.

easwars · 2024-10-21T19:44:54Z

internal/testutils/balancer.go

@@ -32,6 +32,7 @@ import (

 // TestSubConn implements the SubConn interface, to be used in tests.
 type TestSubConn struct {
+	balancer.SubConn


Funny that we ourselves didn't embed the interface in our tests.

dfawley · 2024-10-21T20:26:38Z

Do you think it's okay to expect users manually edit their mocks?

Probably not if they're generated, unless they're also checked in, and then...maybe. How many instances of this exist? Is it feasible to fix all the ones that we're (indirectly) responsible for?

dfawley

LGTM, but we need to wait to check this in until all the internal dependencies are resolved.

…enforce-embedding

arjan-bal · 2024-11-04T16:41:38Z

The PR in grpc-gcp-go is merged: GoogleCloudPlatform/grpc-gcp-go#89
grpc-gcp-go may need to do a release before we're clear to merge this PR.

…rpc#7758)

Enforce embedding the SubConn interface

e72b11c

arjan-bal added the Type: Internal Cleanup Refactors, etc label Oct 18, 2024

arjan-bal added this to the 1.69 Release milestone Oct 18, 2024

arjan-bal requested review from easwars and dfawley October 18, 2024 10:44

arjan-bal assigned easwars and dfawley Oct 18, 2024

arjan-bal changed the title ~~balancer: Enforce embedding the SubConn interface~~ balancer: Enforce embedding the SubConn interface in implementations Oct 18, 2024

dfawley approved these changes Oct 18, 2024

View reviewed changes

dfawley assigned arjan-bal and unassigned dfawley Oct 18, 2024

Fix grammar

acca0c6

arjan-bal assigned dfawley and unassigned arjan-bal Oct 21, 2024

easwars reviewed Oct 21, 2024

View reviewed changes

easwars approved these changes Oct 22, 2024

View reviewed changes

easwars assigned arjan-bal and unassigned easwars Oct 22, 2024

arjan-bal mentioned this pull request Oct 25, 2024

Embed balancer.SubConn in generated MockSubConn GoogleCloudPlatform/grpc-gcp-go#89

Merged

dfawley approved these changes Oct 25, 2024

View reviewed changes

dfawley removed their assignment Oct 25, 2024

Merge commit 'd66fc3a1efa1dfb33dfedf9760528f1ac2b923b6' into subconn-…

152cbce

…enforce-embedding

arjan-bal mentioned this pull request Nov 4, 2024

Flaky test: TestResolverRemovedWithRPCs #7799

Closed

arjan-bal merged commit 43ee172 into grpc:master Nov 5, 2024
15 checks passed

arjan-bal deleted the subconn-enforce-embedding branch November 5, 2024 17:52

arjan-bal added the Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. label Nov 6, 2024

misvivek pushed a commit to misvivek/grpc-go that referenced this pull request Nov 7, 2024

balancer: Enforce embedding the SubConn interface in implementations (g…

cb67d23

…rpc#7758)

dfawley mentioned this pull request Nov 13, 2024

balancer: fix SubConn embedding requirement to not recommend a nil panic hazard #7840

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

balancer: Enforce embedding the SubConn interface in implementations #7758

balancer: Enforce embedding the SubConn interface in implementations #7758

arjan-bal commented Oct 18, 2024 •

edited by dfawley

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

dfawley left a comment

dfawley Oct 18, 2024

arjan-bal Oct 21, 2024

arjan-bal commented Oct 21, 2024 •

edited

Loading

easwars commented Oct 21, 2024

easwars Oct 21, 2024

dfawley commented Oct 21, 2024

dfawley left a comment

arjan-bal commented Nov 4, 2024

balancer: Enforce embedding the SubConn interface in implementations #7758

balancer: Enforce embedding the SubConn interface in implementations #7758

Conversation

arjan-bal commented Oct 18, 2024 • edited by dfawley Loading

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

dfawley left a comment

Choose a reason for hiding this comment

dfawley Oct 18, 2024

Choose a reason for hiding this comment

arjan-bal Oct 21, 2024

Choose a reason for hiding this comment

arjan-bal commented Oct 21, 2024 • edited Loading

easwars commented Oct 21, 2024

easwars Oct 21, 2024

Choose a reason for hiding this comment

dfawley commented Oct 21, 2024

dfawley left a comment

Choose a reason for hiding this comment

arjan-bal commented Nov 4, 2024

arjan-bal commented Oct 18, 2024 •

edited by dfawley

Loading

codecov bot commented Oct 18, 2024 •

edited

Loading

arjan-bal commented Oct 21, 2024 •

edited

Loading