Skip to content

internal/transport: Wait for server goroutines to exit during shutdown in test #8306

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented May 9, 2025

Internal bug: b/416700146

Presently the PingPong tests don't wait for server goroutines to finish work before the return. This causes data races when the next test tries to update the GRPCLogger. Closing the server isn't sufficient.

// Close starts shutting down the http2Server transport.
// TODO(zhaoq): Now the destruction is not blocked on any pending streams. This
// could cause some resource issue. Revisit this later.
func (t *http2Server) Close(err error) {

RELEASE NOTES: N/A

@arjan-bal arjan-bal added this to the 1.73 Release milestone May 9, 2025
@arjan-bal arjan-bal requested review from easwars and dfawley May 9, 2025 15:50
@arjan-bal arjan-bal added Type: Testing Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. labels May 9, 2025
Copy link

codecov bot commented May 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.35%. Comparing base (515f377) to head (6c43100).
Report is 21 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8306      +/-   ##
==========================================
+ Coverage   82.14%   82.35%   +0.20%     
==========================================
  Files         417      419       +2     
  Lines       41344    41988     +644     
==========================================
+ Hits        33961    34578     +617     
- Misses       5957     5963       +6     
- Partials     1426     1447      +21     

see 50 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

})
go func() {
transport.HandleStreams(ctx, func(s *ServerStream) {
go h.handleStreamMisbehave(t, s)
Copy link
Member

Choose a reason for hiding this comment

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

Should this (and similar) be outside the other goroutine now? Or does HandleStreams track this and not return until all streams it's handling are done?

@dfawley dfawley assigned arjan-bal and unassigned easwars and dfawley May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. Type: Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants