🐛 fix: Add BodyStream() logic to adaptor.FiberHandler middleware#3799
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds streaming support to the HTTP→Fiber adaptor: when a response exposes a BodyStream it streams in 32 KiB pooled buffers, writing chunks to the net/http ResponseWriter and flushing after each chunk; otherwise it writes the full body. Tests added for flush and interrupted streaming. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant NetHTTP as net/http Handler
participant Adaptor as adaptor.FiberHandler
participant Fiber as Fiber Handler
participant BodyStream
Client->>NetHTTP: HTTP request
NetHTTP->>Adaptor: ServeHTTP(w,r)
Adaptor->>Fiber: invoke Fiber handler
alt BodyStream present
Fiber-->>Adaptor: Response with BodyStream
loop stream chunks
Adaptor->>BodyStream: Read into pooled 32KiB buffer
BodyStream-->>Adaptor: Chunk bytes
Adaptor->>NetHTTP: Write chunk to ResponseWriter
Adaptor->>NetHTTP: Flush (http.Flusher)
end
else No BodyStream
Fiber-->>Adaptor: Response with full body
Adaptor->>NetHTTP: Write full body once
end
NetHTTP-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3799 +/- ##
=======================================
Coverage 91.90% 91.91%
=======================================
Files 115 115
Lines 12353 12377 +24
=======================================
+ Hits 11353 11376 +23
Misses 729 729
- Partials 271 272 +1
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:
|
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug where adaptor.FiberHandler middleware didn't support buffered streaming to net/http handlers. The fix adds BodyStream() logic to enable proper streaming when converting Fiber responses to net/http handlers, ensuring that c.SendStreamWriter() works as expected.
- Adds a new buffer pool for efficient stream processing
- Implements streaming logic with chunked reading and flushing
- Maintains backward compatibility for non-streaming responses
adaptor.FiberHandler middlewareadaptor.FiberHandler middleware
There was a problem hiding this comment.
Code Review
This pull request introduces streaming support for adaptor.FiberHandler to correctly handle c.SendStreamWriter(). The implementation uses a sync.Pool for buffer management to stream the response body in chunks. My review includes a couple of suggestions to improve performance and robustness by refining the buffer pool implementation and ensuring resources are properly released.
a1389d5 to
4cf2916
Compare
36c8eb7 to
d4622e7
Compare
|
Note: Since Fiber's client doesn't fully support streaming yet, I used |
|
/gemini review |
|
@grivera64 The Also, why 32*1024 ? Isn't that quite small? |
There was a problem hiding this comment.
Code Review
This pull request introduces streaming support for Fiber to net/http handlers by implementing logic for BodyStream(). The changes are a good step forward, but I've identified a few areas for improvement regarding error handling and code maintainability. Specifically, I've pointed out a potential panic that could crash the server, an unhandled error that could lead to resource leaks, and some suggestions to improve code clarity in both the implementation and the new tests. Addressing these points will make the new functionality more robust and reliable.
For sure, I will add a const to refer to that size.
It is a small default, but we can increase it if that would be better (e.g. 128 KiB). |
| if !ok { | ||
| panic(fmt.Errorf("failed to type-assert to *[%d]byte", bufferSize)) | ||
| } |
There was a problem hiding this comment.
Using panic for a type assertion failure is inappropriate here. This should never happen with a properly configured sync.Pool, but if it does, it would crash the entire application. Consider logging an error and falling back to a regular write operation instead.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
middleware/adaptor/adaptor.go (1)
279-282: Replace panic with graceful error handling.Using
panicin a web handler can crash the server process if the pool returns an unexpected type, potentially causing a denial of service. This represents a programming error within the package that should be logged and handled gracefully instead of crashing.Apply this diff to handle the error gracefully:
- bufPtr, ok := bufferPool.Get().(*[bufferSize]byte) - if !ok { - panic(fmt.Errorf("failed to type-assert to *[%d]byte", bufferSize)) - } + bufPtr, ok := bufferPool.Get().(*[bufferSize]byte) + if !ok { + http.Error(w, utils.StatusMessage(fiber.StatusInternalServerError), fiber.StatusInternalServerError) + return + }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/adaptor/adaptor.go(2 hunks)middleware/adaptor/adaptor_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (Make target:make format).
Run golangci-lint for linting (Make target:make lint).
Rungo vetas part of audit to catch suspicious constructs (Make target:make audit).
Optimize struct field alignment with betteralign (Make target:make betteralign).
Applygopls modernizeto update code patterns (Make target:make modernize).
Files:
middleware/adaptor/adaptor_test.gomiddleware/adaptor/adaptor.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Run the test suite with gotestsum (Make target:make test).
Run benchmarks withgo test(Make target:make benchmark).
Files:
middleware/adaptor/adaptor_test.go
🧠 Learnings (1)
📚 Learning: 2025-05-08T08:14:37.302Z
Learnt from: mdelapenya
PR: gofiber/fiber#3434
File: app.go:623-636
Timestamp: 2025-05-08T08:14:37.302Z
Learning: In the gofiber/fiber framework, service startup failures should panic rather than allowing the application to continue running with degraded functionality, as this is the agreed-upon design decision.
Applied to files:
middleware/adaptor/adaptor.go
🧬 Code graph analysis (1)
middleware/adaptor/adaptor_test.go (2)
constants.go (3)
StatusTeapot(91-91)MethodGet(5-5)NetworkTCP4(292-292)middleware/adaptor/adaptor.go (1)
FiberHandlerFunc(161-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: repeated
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
🔇 Additional comments (3)
middleware/adaptor/adaptor.go (2)
32-38: LGTM! Buffer pool is well-designed.The constant definition and array pointer pooling align with best practices. The 32 KiB buffer size matches Go's
io.Copydefault, which is a sensible choice for streaming operations.
286-298: LGTM! Streaming loop is well-implemented.The loop properly handles read/write errors and flushes after each write, ensuring data is delivered to the client incrementally. The error handling addresses previous review feedback.
middleware/adaptor/adaptor_test.go (1)
510-510: LGTM! No-op Flush implementation is appropriate.Adding a no-op
Flush()method allowsnetHTTPResponseWriterto satisfy thehttp.Flusherinterface for testing streaming behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
middleware/adaptor/adaptor_test.go (1)
704-732: Consider reducing sleep duration for faster tests.The 200ms sleep simulates a long operation, which is valid for testing the streaming pattern. However, if test suite performance becomes a concern, consider reducing this to 50-100ms—streaming behavior will still be validated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
middleware/adaptor/adaptor_test.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (Make target:make format).
Run golangci-lint for linting (Make target:make lint).
Rungo vetas part of audit to catch suspicious constructs (Make target:make audit).
Optimize struct field alignment with betteralign (Make target:make betteralign).
Applygopls modernizeto update code patterns (Make target:make modernize).
Files:
middleware/adaptor/adaptor_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Run the test suite with gotestsum (Make target:make test).
Run benchmarks withgo test(Make target:make benchmark).
Files:
middleware/adaptor/adaptor_test.go
🧬 Code graph analysis (1)
middleware/adaptor/adaptor_test.go (2)
constants.go (3)
StatusTeapot(91-91)MethodGet(5-5)NetworkTCP4(292-292)middleware/adaptor/adaptor.go (1)
FiberHandlerFunc(161-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (1)
middleware/adaptor/adaptor_test.go (1)
510-511: LGTM: Appropriate no-op for test mock.The no-op
Flush()implementation is correct for this test helper type. Note thatTest_FiberHandler_WithSendStreamWriteruses this mock, so it validates the streaming logic path but not the actual flush-to-wire behavior.Test_FiberHandler_WithInterruptedSendStreamWriteraddresses this by using a real HTTP server where flushing matters.
|
@grivera64 32kb is fine then. We can revisit in the future |
…)/fiber-handler-stream-writer-support
Description
In the opposite direction of #3421, there is also a bug where adaptor.FiberHandler also does not support buffered streaming directly to a net/http handler.
This PR adds
BodyStream()logic to ensure thatc.SendStreamWriter()works as expected.Fixes #3798
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Important
This PR is a DRAFT. The following functionality is a WIP:[X] AddBodyStream()logic[x] Add unit tests