grpc-go icon indicating copy to clipboard operation
grpc-go copied to clipboard

[http_util/bufWriter] fast-fail on error returned from flushKeepBuffer()

Open veshij opened this issue 1 year ago • 1 comments

fixes https://github.com/grpc/grpc-go/issues/7389

veshij avatar Jul 05 '24 19:07 veshij

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.42%. Comparing base (bdd707e) to head (877dd6f). Report is 38 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #7394    +/-   ##
========================================
  Coverage   81.42%   81.42%            
========================================
  Files         348      354     +6     
  Lines       26744    27083   +339     
========================================
+ Hits        21775    22053   +278     
- Misses       3779     3817    +38     
- Partials     1190     1213    +23     
Files Coverage Δ
internal/transport/http_util.go 93.30% <100.00%> (+6.89%) :arrow_up:

... and 41 files with indirect coverage changes

codecov[bot] avatar Jul 05 '24 19:07 codecov[bot]

Also, please respond to individual comments. That helps reviewers to keep track of the individual changes that they requested (and eventually mark them as resolved). Thanks.

easwars avatar Jul 16 '24 22:07 easwars

Looks like a bunch of tests are failing. PTAL.

easwars avatar Jul 23 '24 14:07 easwars

looking at failed tests

veshij avatar Jul 24 '24 17:07 veshij

@veshij any update?

dfawley avatar Jul 30 '24 16:07 dfawley

@veshij any update?

@dfawley *bufWriter.Write implementation can completely skip flushing if w.offset >= w.batchSize condition is never met. In this case once this buffer is reused later on - flushKeepBuffer() would return more bytes than was initially sent to Write() call. Thus we can't reuse value returned from flushKeepBuffer(). One option would be to leave total written bytes accounting as is (always written += copied), or force-flush buffer at the end of Write call (which is likely too expensive).

Updated PR with the first option.

veshij avatar Aug 01 '24 02:08 veshij

@printchard : FYI

easwars avatar Aug 07 '24 18:08 easwars