refactor(transport): extract shared stream state handling logic in loopyWriter.processData()

[Exgene]

Fixes: #9117

Refactors the duplicated post write stream state handling in loopyWriter.processData() present in internal/transport/controlbuf.go into a shared helper l.updateStreamAfterWrite(). Both empty and non empty DATA frame paths now share the same stream state handling logic.

This change also adds the stream quota check (int(l.oiws) - str.bytesOutStanding) to the empty frame path. This is safe because empty DATA frames send 0 bytes, so bytesOutStanding doesn't change.

RELEASE NOTES: n/a

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Exgene / name: Kausthubh J Rao (dd7c7b5)

[codecov]

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.13%. Comparing base (7c47d2a) to head (dd7c7b5).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
internal/transport/controlbuf.go	72.72%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9125      +/-   ##
==========================================
- Coverage   83.21%   83.13%   -0.08%     
==========================================
  Files         413      413              
  Lines       33482    33479       -3     
==========================================
- Hits        27863    27834      -29     
- Misses       4212     4224      +12     
- Partials     1407     1421      +14     

Files with missing lines	Coverage Δ
internal/transport/controlbuf.go	91.17% <72.72%> (+1.61%)	⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Exgene]

how do i add labels? i can't really add it from my side on the sidebar, and it wasn't present in the contributing guidelines either.

[easwars]

@arjan-bal : Could you please take a look at this one? Thanks.

[arjan-bal]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the processData method in internal/transport/controlbuf.go by extracting duplicated stream update logic into a new helper method called updateStreamAfterWrite. This helper method now centralizes the logic for updating stream states, handling trailers, and re-enqueuing active streams after a write operation. I have no feedback to provide as there were no review comments to evaluate.

[arjan-bal]

Some minor comments, otherwise LGTM. Please address them before I add a second reviewer.

[arjan-bal]

nit: Can be simplified as return false, l.updateStreamAfterWrite(str). Same for the updateStreamAfterWrite below.

[arjan-bal]

Can you please leave the comment here?

[Exgene]

happy to add them!

[arjan-bal]

Is suspect we can remove this special handing of empty data frames if we just modify the following check below to not return early:

grpc-go/internal/transport/controlbuf.go

Lines 983 to 986 in 6093108

	if strQuota := int(l.oiws) - str.bytesOutStanding; strQuota <= 0 { // stream-level flow control.
	str.state = waitingOnStreamQuota
	return false, nil
	} else if maxSize > strQuota {

It should be something similar to:

isEmpty := len(dataItem.h) == 0 && reader.Remaining() == 0
if strQuota <= 0 && !isEmpty {
    // return early
}

Can you check if this is possible?

[Exgene]

sure let me check that

Exploring further simplification.

[arjan-bal]

happy to add them!

To clarify, I'll add the second reviewer after you've addressed the pending comments.

[Exgene]

@arjan-bal I have refactored it accordingly, I think there is this one case where the frame is empty and the value of strQuota could become negative? hence i clamped it with max(strQuota, 0).

I also refactored

else if maxSize > strQuota {
        maxSize = strQuota
}

with simpler

maxSize = min(maxSize, int(l.sendQuota)) 

And finally in the empty frame case should onEachWrite() run? i have gated that as well for now.

[Exgene]

Also not sure why that test case is failing.