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.

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

transport: Fix reporting of bytes read while reading headers #7660

Merged
merged 1 commit into from
Sep 23, 2024
Merged

transport: Fix reporting of bytes read while reading headers #7660

merged 1 commit into from
Sep 23, 2024

Conversation

arjan-bal
Copy link
Contributor

@arjan-bal arjan-bal commented Sep 22, 2024 •
edited
Loading

Fixes: #7641

Background

The headers are being read incrementally here:

grpc-go/internal/transport/transport.go

Lines 559 to 579 in 8ea3460

func (s *Stream) ReadHeader(header []byte) (err error) {
// Don't request a read if there was an error earlier
if er := s.trReader.er; er != nil {
return er
}
s.requestRead(len(header))
for len(header) != 0 {
n, err := s.trReader.ReadHeader(header)
header = header[n:]
if len(header) == 0 {
err = nil
}
if err != nil {
if n > 0 && err == io.EOF {
err = io.ErrUnexpectedEOF
}
return err
}
}
return nil
}

We keep filling the underlying header slice until its full, but we do so by moving the start of the slice forward. In the following function that is called by the function above to perform reads and update the flow control window, we do t.windowHandler(len(header)).

grpc-go/internal/transport/transport.go

Lines 622 to 630 in 8ea3460

func (t *transportReader) ReadHeader(header []byte) (int, error) {
n, err := t.reader.ReadHeader(header)
if err != nil {
t.er = err
return 0, err
}
t.windowHandler(len(header))
return n, nil
}

The code has read n bytes, but it says it read len(header) bytes. len(header) is the total number of bytes that remain to be read, which is not always equal the the bytes there were read. I was able to get the repro test to pass after changing the line to t.windowHandler(n).

RELEASE NOTES:

  • transport: Fix a bug causing stream failures due to miscalculation of the flow control window in both clients and servers.

@arjan-bal arjan-bal added Type: Bug Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. labels Sep 22, 2024
@arjan-bal arjan-bal added this to the 1.68 Release milestone Sep 22, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.80%. Comparing base (6c48e47) to head (13c93c4).
Report is 2 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7660      +/-   ##
==========================================
- Coverage   81.93%   81.80%   -0.13%     
==========================================
  Files         361      361              
  Lines       27818    27821       +3     
==========================================
- Hits        22792    22759      -33     
- Misses       3837     3861      +24     
- Partials     1189     1201      +12
Files with missing lines Coverage Δ
internal/transport/transport.go 92.69% <100.00%> (+0.76%) ⬆️

... and 22 files with indirect coverage changes

easwars
easwars approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Nice find.

@easwars
Copy link
Contributor

easwars commented Sep 23, 2024

@PapaCharlie : FYI

@easwars easwars assigned arjan-bal and unassigned easwars Sep 23, 2024
@arjan-bal arjan-bal merged commit bcf9171 into grpc:master Sep 23, 2024
14 checks passed
@arjan-bal arjan-bal deleted the fix-flow-control branch September 23, 2024 16:09
@arjan-bal arjan-bal mentioned this pull request Sep 23, 2024
Closed
@PapaCharlie
Copy link
Contributor

LGTM this makes sense, nice find!

arjan-bal added a commit to arjan-bal/grpc-go that referenced this pull request Sep 24, 2024
@arjan-bal
transport: Fix reporting of bytes read while reading headers (grpc#7660)
7398679
@arjan-bal arjan-bal mentioned this pull request Sep 24, 2024
Merged
arjan-bal added a commit that referenced this pull request Sep 24, 2024
@arjan-bal
transport: Fix reporting of bytes read while reading headers (#7660) (#…
935f8cb 
…7667)
arjan-bal added a commit to arjan-bal/grpc-go that referenced this pull request Sep 24, 2024
@arjan-bal
transport: Fix reporting of bytes read while reading headers (grpc#7660)
d53bc9a
@arjan-bal arjan-bal mentioned this pull request Sep 24, 2024
Merged
arjan-bal added a commit that referenced this pull request Sep 25, 2024
@arjan-bal
transport: Fix reporting of bytes read while reading headers (#7660) (#…
bcbf356 
…7668)
janardhanvissa pushed a commit to janardhanvissa/grpc-go that referenced this pull request Oct 1, 2024
@arjan-bal @janardhankrishna-sai
transport: Fix reporting of bytes read while reading headers (grpc#7660)
a5ae081
@arjan-bal arjan-bal mentioned this pull request Oct 24, 2024
Closed
@tbg tbg mentioned this pull request Nov 27, 2024
Draft
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@easwars easwars easwars approved these changes

@dfawley dfawley Awaiting requested review from dfawley

Assignees

@arjan-bal arjan-bal

Labels
Area: Transport Includes HTTP/2 client/server and HTTP server handler transports and advanced transport features. Type: Bug
Projects
None yet
Milestone
1.68 Release
Development

Successfully merging this pull request may close these issues.

Long running streams fail
3 participants
@arjan-bal @easwars @PapaCharlie