[WIP] Implement proper payload chunked encoding in HTTP api_v3 #6479
Conversation
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (72.22%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6479 +/- ##
==========================================
- Coverage 96.23% 96.15% -0.09%
==========================================
Files 368 375 +7
Lines 20978 21433 +455
==========================================
+ Hits 20189 20608 +419
- Misses 604 629 +25
- Partials 185 196 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
Signed-off-by: chahatsagarmain <[email protected]>
| n, err := resp.Body.Read(buf) | ||
|
|
||
| if n > 0 { | ||
| fullResponse.Write(buf[:n]) |
There was a problem hiding this comment.
if you concatenate the chunks you will end up with invalid JSON
| // Set chunked transfer encoding before any writes | ||
| w.Header().Set("Transfer-Encoding", "chunked") | ||
| w.Header().Set("Content-Encoding", "gzip") | ||
| w.Header().Set("Vary", "Accept-Encoding") |
| } | ||
|
|
||
| // Set chunked transfer encoding before any writes | ||
| w.Header().Set("Transfer-Encoding", "chunked") |
There was a problem hiding this comment.
chunked encoding requires each chunk to be prefixed with its size
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| gz := gzip.NewWriter(w) | ||
| defer gz.Close() |
There was a problem hiding this comment.
The defer gz.Close() placement could cause issues with premature closing. Since it's defined in the middleware handler function, it will execute when that function returns, not when all data has been written. This could lead to incomplete responses.
Consider either:
- Moving the close operation to be explicitly called after all data is written, or
- Implementing a
Close()method on thechunkedGzipWriterstruct that handles proper cleanup
This would ensure the gzip writer remains open until all data has been properly flushed and written to the response.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| n, err := resp.Body.Read(buf) | ||
|
|
There was a problem hiding this comment.
The Read() operation should handle all error cases, not just EOF. Consider updating the error handling to properly catch and respond to any read errors:
n, err := resp.Body.Read(buf)
if err != nil && err != io.EOF {
require.NoError(t, err)
break
}This ensures the test fails appropriately if there's a network issue or other error during response reading, rather than silently continuing.
| n, err := resp.Body.Read(buf) | |
| n, err := resp.Body.Read(buf) | |
| if err != nil && err != io.EOF { | |
| require.NoError(t, err) | |
| break | |
| } |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time. |
github-actions
bot
added
the
stale
2 participants
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test