Repository: grpc/grpc-go

Issue:
Add a test for dependencies of grpc package
We should add a test in `vet.sh` to confirm that the dependencies of grpc don't change without our noticing (i.e. vet should fail if they don't match the current list).

`go list` can pretty easily show dependencies.  We should only be checking our direct dependencies and not our transitive dependencies, since we have no control over the transitive ones.  We should check the `grpc` package, the `xds` package, and any other important packages that are not imported by either of those directly.

Discussion:

Solution patch:
diff --git a/.github/workflows/deps.yml b/.github/workflows/deps.yml
new file mode 100644
index 00000000..c510e0e3
--- /dev/null
+++ b/.github/workflows/deps.yml
@@ -0,0 +1,43 @@
+name: Dependency Changes
+
+# Trigger on PRs.
+on:
+  pull_request:
+
+permissions:
+  contents: read
+
+jobs:
+  # Compare dependencies before and after this PR.
+  dependencies:
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    strategy:
+      fail-fast: true
+
+    steps:
+      - name: Checkout repo
+        uses: actions/checkout@v4
+        with:
+          fetch-depth: 0
+
+      - name: Setup Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: stable
+          cache-dependency-path: "**/*go.sum"
+
+      # Run the commands to generate dependencies before and after and compare.
+      - name: Compare dependencies
+        run: |
+          BEFORE="$(mktemp -d)"
+          AFTER="$(mktemp -d)"
+
+          scripts/gen-deps.sh "${AFTER}"
+          git checkout origin/master
+          scripts/gen-deps.sh "${BEFORE}"
+
+          echo "Comparing dependencies..."
+          # Run grep in a sub-shell since bash does not support ! in the middle of a pipe
+          diff -u0 -r "${BEFORE}" "${AFTER}" | bash -c '! grep -v "@@"'
+          echo "No changes detected."
diff --git a/internal/internal.go b/internal/internal.go
index 20b4dc3d..88900fa9 100644
--- a/internal/internal.go
+++ b/internal/internal.go
@@ -149,6 +149,20 @@ var (
 	// other features, including the CSDS service.
 	NewXDSResolverWithConfigForTesting any // func([]byte) (resolver.Builder, error)
 
+	// NewXDSResolverWithClientForTesting creates a new xDS resolver builder
+	// using the provided xDS client instead of creating a new one using the
+	// bootstrap configuration specified by the supported environment variables.
+	// The resolver.Builder is meant to be used in conjunction with the
+	// grpc.WithResolvers DialOption. The resolver.Builder does not take
+	// ownership of the provided xDS client and it is the responsibility of the
+	// caller to close the client when no longer required.
+	//
+	// Testing Only
+	//
+	// This function should ONLY be used for testing and may not work with some
+	// other features, including the CSDS service.
+	NewXDSResolverWithClientForTesting any // func(xdsclient.XDSClient) (resolver.Builder, error)
+
 	// RegisterRLSClusterSpecifierPluginForTesting registers the RLS Cluster
 	// Specifier Plugin for testing purposes, regardless of the XDSRLS environment
 	// variable.
diff --git a/mem/buffers.go b/mem/buffers.go
index 565ec0a1..ecbf0b9a 100644
--- a/mem/buffers.go
+++ b/mem/buffers.go
@@ -92,7 +92,11 @@ func newBuffer() *buffer {
 //
 // Note that the backing array of the given data is not copied.
 func NewBuffer(data *[]byte, pool BufferPool) Buffer {
-	if pool == nil || IsBelowBufferPoolingThreshold(len(*data)) {
+	// Use the buffer's capacity instead of the length, otherwise buffers may
+	// not be reused under certain conditions. For example, if a large buffer
+	// is acquired from the pool, but fewer bytes than the buffering threshold
+	// are written to it, the buffer will not be returned to the pool.
+	if pool == nil || IsBelowBufferPoolingThreshold(cap(*data)) {
 		return (SliceBuffer)(*data)
 	}
 	b := newBuffer()
diff --git a/scripts/gen-deps.sh b/scripts/gen-deps.sh
new file mode 100755
index 00000000..bc647c4d
--- /dev/null
+++ b/scripts/gen-deps.sh
@@ -0,0 +1,21 @@
+#!/bin/bash
+
+set -e          # Exit on error
+set -o pipefail # Fail a pipe if any sub-command fails.
+
+if [[ "$#" -ne 1 || ! -d "$1" ]]; then
+    echo "Specify a valid output directory as the first parameter."
+    exit 1
+fi
+
+SCRIPTS_DIR="$(dirname "$0")"
+OUTPUT_DIR="$1"
+
+cd "${SCRIPTS_DIR}/.."
+
+git ls-files -- '*.go' | grep -v '\(^\|/\)\(internal\|examples\|benchmark\|interop\|test\|testdata\)\(/\|$\)' | xargs dirname | sort -u | while read d; do
+  pushd "$d" > /dev/null
+  pkg="$(echo "$d" | sed 's;\.;grpc;' | sed 's;/;_;g')"
+  go list -deps . | sort >| "${OUTPUT_DIR}/$pkg"
+  popd > /dev/null
+done
diff --git a/xds/internal/resolver/xds_resolver.go b/xds/internal/resolver/xds_resolver.go
index b5d24e4b..de339a7c 100644
--- a/xds/internal/resolver/xds_resolver.go
+++ b/xds/internal/resolver/xds_resolver.go
@@ -44,10 +44,10 @@ import (
 // xdsresolver.Scheme
 const Scheme = "xds"
 
-// newBuilderForTesting creates a new xds resolver builder using a specific xds
-// bootstrap config, so tests can use multiple xds clients in different
-// ClientConns at the same time.
-func newBuilderForTesting(config []byte) (resolver.Builder, error) {
+// newBuilderWithConfigForTesting creates a new xds resolver builder using a
+// specific xds bootstrap config, so tests can use multiple xds clients in
+// different ClientConns at the same time.
+func newBuilderWithConfigForTesting(config []byte) (resolver.Builder, error) {
 	return &xdsResolverBuilder{
 		newXDSClient: func(name string) (xdsclient.XDSClient, func(), error) {
 			return xdsclient.NewForTesting(xdsclient.OptionsForTesting{Name: name, Contents: config})
@@ -55,9 +55,23 @@ func newBuilderForTesting(config []byte) (resolver.Builder, error) {
 	}, nil
 }
 
+// newBuilderWithClientForTesting creates a new xds resolver builder using the
+// specific xDS client, so that tests have complete control over the exact
+// specific xDS client being used.
+func newBuilderWithClientForTesting(client xdsclient.XDSClient) (resolver.Builder, error) {
+	return &xdsResolverBuilder{
+		newXDSClient: func(string) (xdsclient.XDSClient, func(), error) {
+			// Returning an empty close func here means that the responsibility
+			// of closing the client lies with the caller.
+			return client, func() {}, nil
+		},
+	}, nil
+}
+
 func init() {
 	resolver.Register(&xdsResolverBuilder{})
-	internal.NewXDSResolverWithConfigForTesting = newBuilderForTesting
+	internal.NewXDSResolverWithConfigForTesting = newBuilderWithConfigForTesting
+	internal.NewXDSResolverWithClientForTesting = newBuilderWithClientForTesting
 
 	rinternal.NewWRR = wrr.NewRandom
 	rinternal.NewXDSClient = xdsclient.New



Test patch:
diff --git a/internal/testutils/wrappers.go b/internal/testutils/wrappers.go
index c9b596d8..1770b523 100644
--- a/internal/testutils/wrappers.go
+++ b/internal/testutils/wrappers.go
@@ -53,7 +53,7 @@ func (l *ListenerWrapper) Accept() (net.Conn, error) {
 	}
 	closeCh := NewChannel()
 	conn := &ConnWrapper{Conn: c, CloseCh: closeCh}
-	l.NewConnCh.Send(conn)
+	l.NewConnCh.Replace(conn)
 	return conn, nil
 }
 
diff --git a/mem/buffers_test.go b/mem/buffers_test.go
index c1799574..2b0627da 100644
--- a/mem/buffers_test.go
+++ b/mem/buffers_test.go
@@ -98,6 +98,33 @@ func (s) TestBuffer_NewBufferRefAndFree(t *testing.T) {
 	}
 }
 
+func (s) TestBuffer_NewBufferHandlesShortBuffers(t *testing.T) {
+	const threshold = 100
+
+	// Update the pooling threshold, since that's what's being tested.
+	internal.SetBufferPoolingThresholdForTesting.(func(int))(threshold)
+	t.Cleanup(func() {
+		internal.SetBufferPoolingThresholdForTesting.(func(int))(0)
+	})
+
+	// Make a pool with a buffer whose capacity is larger than the pooling
+	// threshold, but whose length is less than the threshold.
+	b := make([]byte, threshold/2, threshold*2)
+	pool := &singleBufferPool{
+		t:    t,
+		data: &b,
+	}
+
+	// Get a Buffer, then free it. If NewBuffer decided that the Buffer
+	// shouldn't get pooled, Free will be a noop and singleBufferPool will not
+	// have been updated.
+	mem.NewBuffer(&b, pool).Free()
+
+	if pool.data != nil {
+		t.Fatalf("Buffer not returned to pool")
+	}
+}
+
 func (s) TestBuffer_FreeAfterFree(t *testing.T) {
 	buf := newBuffer([]byte("abcd"), mem.NopBufferPool{})
 	if buf.Len() != 4 {
diff --git a/xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go b/xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go
index b82e6d09..89f3d1bc 100644
--- a/xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go
+++ b/xds/internal/balancer/clusterresolver/e2e_test/aggregate_cluster_test.go
@@ -112,7 +112,9 @@ func (s) TestAggregateCluster_WithTwoEDSClusters(t *testing.T) {
 				return nil
 			}
 			if len(req.GetResourceNames()) == 0 {
-				// This is the case for ACKs. Do nothing here.
+				// This happens at the end of the test when the grpc channel is
+				// being shut down and it is no longer interested in xDS
+				// resources.
 				return nil
 			}
 			select {
@@ -489,11 +491,15 @@ func (s) TestAggregateCluster_WithEDSAndDNS(t *testing.T) {
 			if req.GetTypeUrl() != version.V3EndpointsURL {
 				return nil
 			}
-			if len(req.GetResourceNames()) > 0 {
-				select {
-				case edsResourceCh <- req.GetResourceNames()[0]:
-				default:
-				}
+			if len(req.GetResourceNames()) == 0 {
+				// This happens at the end of the test when the grpc channel is
+				// being shut down and it is no longer interested in xDS
+				// resources.
+				return nil
+			}
+			select {
+			case edsResourceCh <- req.GetResourceNames()[0]:
+			default:
 			}
 			return nil
 		},