transport: add timeout for writing GOAWAY on http2Client.Close() #7371

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.

Sign up for GitHub

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

Jump to bottom

Merged

dfawley merged 19 commits into grpc:master from aranjans:aranjans_7314

Aug 16, 2024

Contributor

aranjans commented Jul 1, 2024 •

edited

Loading

fixes #7314

Problem

In gRPC 1.64, we released a feature "Send GOAWAY to server on Client Transport Shutdown", post which in case of network hang when we are not able to write GOAWAY frame from client, closing connections will take a very long time.

What does this PR fix

This PR fixes this issue by having a timeout for loopyWriter to write GOAWAY so that http2Client.Close() doesn't wait for too long.

RELEASE NOTES:

transport/http2_client: fix waiting on http2Client.Close() in case of network hang for writing GOAWAY.

aranjans requested a review from purnesh42H

July 1, 2024 08:04

aranjans added the Type: Bug label

aranjans added this to the 1.66 Release milestone

codecov bot commented Jul 1, 2024 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.85%. Comparing base (ced812e) to head (fad99ba).
Report is 15 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7371      +/-   ##
==========================================
+ Coverage   81.58%   81.85%   +0.27%     
==========================================
  Files         358      360       +2     
  Lines       27388    27539     +151     
==========================================
+ Hits        22345    22543     +198     
+ Misses       3828     3796      -32     
+ Partials     1215     1200      -15

Files	Coverage Δ
internal/transport/http2_client.go	`91.90% <100.00%> (-0.16%)`	⬇️

... and 38 files with indirect coverage changes

purnesh42H requested a review from arjan-bal

July 2, 2024 03:58

purnesh42H self-assigned this

purnesh42H reviewed

View reviewed changes

internal/transport/transport_test.go Outdated Show resolved Hide resolved

internal/transport/transport_test.go Outdated Show resolved Hide resolved

internal/transport/http2_client.go Outdated Show resolved Hide resolved

internal/transport/transport_test.go Outdated Show resolved Hide resolved

internal/transport/transport_test.go Outdated Show resolved Hide resolved

aranjans removed the request for review from arjan-bal

July 2, 2024 06:24

aranjans force-pushed the aranjans_7314 branch from 6338333 to 97ef025 Compare

July 2, 2024 07:51

purnesh42H reviewed

View reviewed changes

internal/transport/http2_client.go Outdated Show resolved Hide resolved

purnesh42H reviewed

View reviewed changes

internal/transport/transport_test.go Outdated Show resolved Hide resolved

internal/transport/http2_client.go Outdated Show resolved Hide resolved

internal/transport/transport_test.go Outdated Show resolved Hide resolved

aranjans force-pushed the aranjans_7314 branch 2 times, most recently from 8e61c11 to 3ea4a82 Compare

July 2, 2024 10:21

purnesh42H reviewed

View reviewed changes

internal/transport/transport_test.go Outdated Show resolved Hide resolved

purnesh42H reviewed

View reviewed changes

internal/transport/transport_test.go Outdated Show resolved Hide resolved

purnesh42H reviewed

View reviewed changes

internal/transport/transport_test.go Outdated Show resolved Hide resolved

dfawley assigned aranjans and unassigned purnesh42H

aranjans force-pushed the aranjans_7314 branch 2 times, most recently from c81d450 to 3684229 Compare

July 3, 2024 10:21

aranjans assigned purnesh42H and unassigned aranjans

purnesh42H reviewed

View reviewed changes

internal/transport/transport_test.go Outdated Show resolved Hide resolved

purnesh42H reviewed

View reviewed changes

internal/transport/transport_test.go Outdated Show resolved Hide resolved

purnesh42H reviewed

View reviewed changes

internal/transport/http2_client.go Show resolved Hide resolved

purnesh42H assigned aranjans and unassigned purnesh42H

aranjans force-pushed the aranjans_7314 branch 2 times, most recently from 3363377 to 87da73c Compare

July 5, 2024 06:42

purnesh42H reviewed

View reviewed changes

internal/transport/transport_test.go Outdated Show resolved Hide resolved

dfawley assigned aranjans and unassigned dfawley

aranjans added 2 commits

August 14, 2024 16:19


          fixed nits

a7a7bb5


          Set write deadline for net.Conn

9636c56

Member

dfawley commented Aug 15, 2024

I'd really like to wrap this up before we cut the release branch. Will that be possible?

aranjans force-pushed the aranjans_7314 branch from 411a244 to 15f6613 Compare

August 16, 2024 07:24


          remove timer for hc.Write

aebe562

aranjans force-pushed the aranjans_7314 branch from 15f6613 to aebe562 Compare

August 16, 2024 07:28

arjan-bal modified the milestones: 1.66 Release, 1.67 Release

aranjans assigned dfawley and unassigned aranjans

dfawley reviewed

View reviewed changes

internal/transport/transport_test.go Outdated

-              		case <-hc.hangConn:
-              		case <-timer.C:
-              		}
+              	if n == goAwayFrameSize { // hang the conn after the goAway is received

Member

dfawley Aug 16, 2024

Why did you remove the atomic?

This is worse, since you're just assuming that: 1. Nothing else written will be the size of the GOAWAY frame, and 2. the GOAWAY will be written to the connection, by itself, in a single operation.

dfawley reviewed

View reviewed changes

internal/transport/transport_test.go Outdated

    
              }

              func (hc *hangingConn) Write(b []byte) (n int, err error) {

              	n, err = hc.Conn.Write(b)

              	if n == goAwayFrameSize { // hang the conn after the goAway is received

              	if hc.startHanging.Load() == true {

Member

dfawley Aug 16, 2024

if x == true --> if x, please

Contributor Author

aranjans Aug 16, 2024

done

aranjans force-pushed the aranjans_7314 branch from 2972676 to 74efc67 Compare

August 16, 2024 17:38

dfawley reviewed

View reviewed changes

internal/transport/transport_test.go Outdated Show resolved Hide resolved

internal/transport/transport_test.go Outdated Show resolved Hide resolved

internal/transport/transport_test.go Outdated Show resolved Hide resolved

internal/transport/transport_test.go Outdated Show resolved Hide resolved

aranjans force-pushed the aranjans_7314 branch from 74efc67 to b52c896 Compare

August 16, 2024 17:47


          Revert the atomic boolean for hangingConn.Write()

fad99ba

aranjans force-pushed the aranjans_7314 branch from b52c896 to fad99ba Compare

August 16, 2024 17:48

dfawley approved these changes

View reviewed changes

dfawley changed the title ~~transport/http2_client: add timeout for writing GOAWAY on client.Close()~~ transport: add timeout for writing GOAWAY on http2Client.Close()

dfawley merged commit 4e29cc6 into grpc:master

13 checks passed

infovivek2020 pushed a commit to infovivek2020/grpc-go that referenced this pull request


          transport: add timeout for writing GOAWAY on http2Client.Close() (grp…

ded7eb8

…c#7371)

aranjans added a commit to aranjans/grpc-go that referenced this pull request


          transport: add timeout for writing GOAWAY on http2Client.Close() (grp…

55bd4bd

…c#7371)

aranjans mentioned this pull request

cherry-pick: transport: add timeout for writing GOAWAY on http2Client.Close() #7371 #7540

Merged

arvindbr8 pushed a commit that referenced this pull request


          cherry-pick: transport: add timeout for writing GOAWAY on http2Client…

35e915e

….Close() #7371 (#7540)

This was referenced Sep 23, 2024

transport: prevent deadlock in transport Close when GoAway write hangs #7662

Merged

Flaky test: Test/ClientCloseReturnsEarlyWhenGoAwayWriteHangs #7606

Closed

tbg mentioned this pull request

DEPS: upgrade grpc to v1.68.0 cockroachdb/cockroach#136278

Closed

16 tasks

github-actions bot locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels