📚 Doc: correct fasthttpctx Done semantics in context guide#4264
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughDocumentation updates to Fiber's context guide clarifying that ChangesContext Semantics Clarification
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Code Review
This pull request updates the documentation to clarify that Fiber's underlying fasthttp context does not signal client disconnects through the Done channel, which instead only closes during server shutdown. The feedback suggests improving the consistency of external issue links, clarifying the usage of w.Flush() within StreamWriter, and recommending c.RequestCtx() as a parent context for timeouts to ensure server shutdown signals are properly propagated to goroutines.
| provided by `fasthttp` (see [valyala/fasthttp#468](https://github.com/valyala/fasthttp/issues/468) | ||
| and [#965](https://github.com/valyala/fasthttp/issues/965), and the |
There was a problem hiding this comment.
For consistency and clarity, the second issue reference should include the repository name, as it refers to an external repository (valyala/fasthttp).
| provided by `fasthttp` (see [valyala/fasthttp#468](https://github.com/valyala/fasthttp/issues/468) | |
| and [#965](https://github.com/valyala/fasthttp/issues/965), and the | |
| provided by fasthttp (see [valyala/fasthttp#468](https://github.com/valyala/fasthttp/issues/468) | |
| and [valyala/fasthttp#965](https://github.com/valyala/fasthttp/issues/965), and the |
| the section below) or fall back to detecting failed `w.Flush()` writes on | ||
| streaming responses. |
There was a problem hiding this comment.
| Note that `fasthttp` does not surface client disconnects through the request | ||
| context, so the timeout (not a client-side cancellation signal) is what bounds | ||
| the work here. |
There was a problem hiding this comment.
While it is correct that client disconnects are not surfaced, the server shutdown signal (from c.RequestCtx().Done()) is available. To ensure goroutines also terminate during a graceful server shutdown, it is recommended to use c.RequestCtx() as the parent context for context.WithTimeout. The current examples (like the one on line 241) use c.Context(), which defaults to context.Background() and does not honor the shutdown signal.
|
@gtoxlili Please fix the wording and tone of the changes. These are official docs, not a forum. |
211e0a9 to
7a5eb8a
Compare
|
@gaby Tightened the wording — dropped the bold emphasis, the inline issue links, and the explanatory asides. Just states the actual semantics now and points to the timeout pattern below. PTAL. |
The "Working with RequestCtx and fasthttpctx" section and the Summary bullet claimed that `RequestCtx.Done` closes when the client connection ends. That is not what fasthttp actually does — `s.done` is the shared server-wide channel and only closes on `Server.Shutdown`. Per-connection disconnect detection is not currently provided by fasthttp. Sources: - valyala/fasthttp `server.go` comment on `RequestCtx.Done`: "RequestCtx.s.done is only closed when the server is shutting down" - valyala/fasthttp#468, gofiber#771, gofiber#965 — upstream maintainers confirm in-handler client disconnect detection is not supported - gofiber#805 (Fenny: "not possible nor efficient with fasthttp sync.pool design") and gofiber#1718 (same Q&A, closed with the same answer) - gofiber#4145 — open proposal asking for the very capability the docs already advertised Updates three spots in docs/guide/context.md: 1. The fasthttpctx paragraph — now describes the actual semantics and points readers to timeout / Flush-based detection for long handlers. 2. The "Context Cancellation with Goroutines" paragraph — drops the misleading "client disconnects" phrasing. 3. The Summary bullet — matches the corrected description. Fixes gofiber#4263
7a5eb8a to
e1af685
Compare
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
The "Working with
RequestCtxandfasthttpctx" section indocs/guide/context.md(introduced in #3677) currently tells readers thatRequestCtx.Donecloses when the client connection ends and that "handlers can detect client disconnects" via this channel. That is not what fasthttp actually does:server.goRequestCtx.Donecarries the explicit comment "RequestCtx.s.doneis only closed when the server is shutting down" —s.doneis the shared server-wide channel, allocated once inServer.Serveand closed once inServer.Shutdown. There is no per-connection cancellation signal.So the docs paragraph contradicts (a) the fasthttp source code, (b) two upstream fasthttp maintainers, (c) a four-year-old Fiber maintainer Q&A answer, and (d) an open Fiber proposal asking for the very capability the docs claim is already there.
Fixes #4263
Changes introduced
Three spots in
docs/guide/context.md:fasthttpctxparagraph (~L100): now describes the actual semantics (server-shutdown-only, shared channel, no client-disconnect signal) and points readers tocontext.WithTimeoutorFlush-error detection for long handlers.No code changes; no API changes.
docs/guide/context.md— three passages corrected, no new sections.Type of change
Checklist
/docs/directory.