Skip to content
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

internal/transport: convert ConnectionError to Unavailable status when writing headers #6891

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

mustafasen81
Copy link
Contributor

@mustafasen81 mustafasen81 commented Dec 26, 2023

The current behavior when server sees a connection failure, when trying to write headers, is to return UNKNOWN status code. This PR changes the behavior to instead return UNAVAILABLE in this case, which is what would be generated by the client if it saw the connection failure when making the RPC.

Fixes #6845

RELEASE NOTES:

  • server: change some stream operations to return UNAVAILABLE instead of UNKNOWN when underlying connection is broken

Copy link

linux-foundation-easycla bot commented Dec 26, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

codecov bot commented Dec 26, 2023

Codecov Report

Merging #6891 (0b23381) into master (3a8270f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6891      +/-   ##
==========================================
- Coverage   83.76%   83.75%   -0.01%     
==========================================
  Files         287      287              
  Lines       30832    30835       +3     
==========================================
+ Hits        25825    25827       +2     
- Misses       3953     3954       +1     
  Partials     1054     1054              
Files Coverage Δ
internal/transport/http2_server.go 89.69% <100.00%> (+0.02%) ⬆️

... and 12 files with indirect coverage changes

@easwars
Copy link
Contributor

easwars commented Dec 26, 2023

@mustafasen81 : Could you please sign the CLA before we proceed and review your PR. Thanks.

@mustafasen81
Copy link
Contributor Author

@easwars, I signed the CLA.

@easwars easwars changed the title Convert ConnectionError to Unavailable status in WriteHeader internal/transport: convert ConnectionError to Unavailable status when writing headers Dec 27, 2023
@easwars easwars added this to the 1.61 Release milestone Dec 27, 2023
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
internal/transport/transport_test.go Outdated Show resolved Hide resolved
@easwars
Copy link
Contributor

easwars commented Dec 28, 2023

@dfawley : for second set of eyes

@ginayeh ginayeh assigned zasweq and unassigned dfawley Jan 3, 2024
@ginayeh ginayeh requested a review from dfawley January 3, 2024 18:09
@ginayeh ginayeh assigned dfawley and unassigned zasweq and mustafasen81 Jan 3, 2024
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM; just some minor stuff. Thank you for the PR!

Comment on lines 2155 to 2165
if len(server.conns) != 1 {
t.Fatalf("Server has %d connections from the client, want 1", len(server.conns))
}

// Get the server transport for the connecton to the client.
var serverTransport *http2Server
server.mu.Lock()
for k := range server.conns {
serverTransport = k.(*http2Server)
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we not need to take server.mu before reading server.conns (the len check on 2155)?

Why a for loop instead of simply serverTransport := server.conns[0].(*http2Server) (which I'd feel better about with server.mu held for the duration of checking len(server.conns) and assigning serverTransport)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

server.conns is of type map[ServerTransport]net.Conn so we cannot access a key by an index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

server.mu moved before reading server.conns (the len check on 2155)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, of course...my mistake.

internal/transport/transport_test.go Outdated Show resolved Hide resolved
internal/transport/transport_test.go Outdated Show resolved Hide resolved
@mustafasen81
Copy link
Contributor Author

mustafasen81 commented Jan 10, 2024

I rebased my commits on the latest master but Testing / vet-proto is still failing. I couldn't understand the reason. Could you help me? @dfawley, @easwars

@dfawley
Copy link
Member

dfawley commented Jan 10, 2024

I rebased my commits on the latest master but Testing / vet-proto is still failing.

This is an optional checker that lets us know we need to update our generated code. #6916 should take care of it, and there's no need to update your PR.

@dfawley dfawley merged commit 6ce73bf into grpc:master Jan 10, 2024
13 of 14 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gRPC server stream returns status of Unknown when connection to the client closed
4 participants