Repository: grpc/grpc-go
File: internal/transport/transport_test.go (Go)

Code before:
			}

			// Wait for the stream to be created on the server side
			var serverStream *ServerStream
			waitWhileTrue(t, func() (bool, error) {
				serverTransport.mu.Lock()
				defer serverTransport.mu.Unlock()
				for _, v := range serverTransport.activeStreams {
					if v.id == clientStream.id {
						serverStream = v
						return false, nil
					}
				}
				return true, nil
			})

			if serverStream == nil {
				t.Fatalf("Server stream not found for client stream ID %d", clientStream.id)
			}

			// For the NonActiveStream test, remove the stream from activeStreams
			if test.removeFromActive {
				serverTransport.mu.Lock()
				delete(serverTransport.activeStreams, serverStream.id)
				serverTransport.mu.Unlock()
			}

			initialSucceeded := serverTransport.channelz.SocketMetrics.StreamsSucceeded.Load()
			initialFailed := serverTransport.channelz.SocketMetrics.StreamsFailed.Load()

			serverTransport.deleteStream(serverStream, test.eosReceived)
			serverTransport.deleteStream(serverStream, test.eosReceived)
			serverTransport.deleteStream(serverStream, test.eosReceived)

			finalSucceeded := serverTransport.channelz.SocketMetrics.StreamsSucceeded.Load()
			finalFailed := serverTransport.channelz.SocketMetrics.StreamsFailed.Load()

			gotSucceededDelta := finalSucceeded - initialSucceeded
			gotFailedDelta := finalFailed - initialFailed

			if gotSucceededDelta != test.wantStreamSucceeded {
				t.Errorf("StreamsSucceeded: got delta %d, want %d", gotSucceededDelta, test.wantStreamSucceeded)
			}
			if gotFailedDelta != test.wantStreamFailed {
				t.Errorf("StreamsFailed: got delta %d, want %d", gotFailedDelta, test.wantStreamFailed)
			}

			// Verify stream was removed from activeStreams (unless it was pre-removed for NonActiveStream test)
			if !test.removeFromActive {
				serverTransport.mu.Lock()
				_, exists := serverTransport.activeStreams[serverStream.id]

Review comment (suggestion):
I think we should not access the `activeStreams` map directly. We should rely on the first call to `serverTransport.deleteStream` to delete the stream. After the first call to `serverTransport.deleteStream`, we should assert that the metric counts are as expected.

After the first call to `serverTransport.deleteStream`, we can assume that the stream is deleted from the `activeStreams` map. We can call `serverTransport.deleteStream` a couple of times more and verify that the metric values haven't changed.

This would allow us to get rid of the `removeFromActive` test param since that case is already covered by the assertions made in the previous paragraph.

Code after:
					if v.id == clientStream.id {
						serverStream = v
						return false, nil
					}
				}
				return true, nil
			})

			if serverStream == nil {
				t.Fatalf("Server stream not found for client stream ID %d", clientStream.id)
			}

			// First call to deleteStream should remove the stream from activeStreams and update metrics
			serverTransport.deleteStream(serverStream, test.eosReceived)

			// Check metrics after first deleteStream call
			streamsSucceeded := serverTransport.channelz.SocketMetrics.StreamsSucceeded.Load()
			streamsFailed := serverTransport.channelz.SocketMetrics.StreamsFailed.Load()

			if streamsSucceeded != test.wantStreamSucceeded {
				t.Errorf("After first deleteStream - StreamsSucceeded: got %d, want %d", streamsSucceeded, test.wantStreamSucceeded)
			}
			if streamsFailed != test.wantStreamFailed {
				t.Errorf("After first deleteStream - StreamsFailed: got %d, want %d", streamsFailed, test.wantStreamFailed)
			}

			// Additional calls to deleteStream should not change metrics (stream already deleted)
			serverTransport.deleteStream(serverStream, test.eosReceived)
			serverTransport.deleteStream(serverStream, test.eosReceived)

			// Verify metrics haven't changed after subsequent calls
			additionalStreamsSucceeded := serverTransport.channelz.SocketMetrics.StreamsSucceeded.Load()
			additionalStreamsFailed := serverTransport.channelz.SocketMetrics.StreamsFailed.Load()

			if additionalStreamsSucceeded != test.wantStreamSucceeded {
				t.Errorf("After multiple deleteStream calls - StreamsSucceeded changed: got %d, want %d", additionalStreamsSucceeded, test.wantStreamSucceeded)
			}
			if additionalStreamsFailed != test.wantStreamFailed {
				t.Errorf("After multiple deleteStream calls - StreamsFailed changed: got %d, want %d", additionalStreamsFailed, test.wantStreamFailed)
			}
		})
	}
}