-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
📝 [Proposal]: Add silent request termination #3252
Comments
I think |
I experimented with a few examples of where ExpressJS uses app.get("/", (req, res) => {
res.write("Hello ")
res.connection.end()
res.write("World")
}); // => "Hello " versus app.Get("/", func (c fiber.Ctx) error {
c.WriteString("Hello ")
c.End()
c.WriteString("World")
return nil
}) // => No response Would this cause any confusion to use the same name app.Get("/", func (c fiber.Ctx) error {
return c.SendStreamWriter(func (w *bufio.Writer) {
w.WriteString("Hello ")
w.Flush()
return
w.WriteString("World")
})
}) // => "Hello " |
@grivera64 Hmmm that's confusing. My guess is that connection.end() kills the TCP_CONN to the server. In fasthttp case we dont see the "hello" cause it hasnt been flushed. While express does that on each write() |
@gaby Yes, and since fasthttp doesn't do any work until after the handler finishes, it will not send any data first. To avoid confusion, would it make sense to use a different method name to match the feature's current behavior? Or would it be better to find a different way to achieve the same behavior as |
I wonder if the function |
You compared against express.js, how does it compare to net/http? |
In http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte("Hello "))
if flusher, ok := w.(http.Flusher); ok {
flusher.Flush()
}
conn, _, err := w.(http.Hijacker).Hijack()
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
conn.Close()
w.Write([]byte("World"))
}) To do this in |
@grivera64 Is there a way to send no data at all? I'm not aure if we should follow Express.js or net/http in this case. |
Actually, it does not send data at all. I was testing http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
fmt.Fprintf(w, "Hello ");
conn, _, err := w.(http.Hijacker).Hijack()
if err != nil {
http.Error(w, "Internal Server Error", http.StatusInternalServerError)
return
}
conn.Close()
fmt.Fprintf(w, "World");
}) // curl address:port => curl: (18) transfer closed with outstanding read data remaining |
Here is a better example that matches with the feature example: // GET "/" (net/http version)
http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
name := r.URL.Query().Get("name")
if name == "" {
conn, _, err := w.(http.Hijacker).Hijack()
if err != nil {
http.Error(w, "Internal Server Error", http.StatusInternalServerError)
return
}
conn.Close()
}
fmt.Fprintf(w, "Hello, %s!", name)
}) // curl http://localhost:3000 => curl: (52) Empty reply from server
// curl http://localhost:3000/\?name=grivera64 => Hello, grivera64! |
In that case, if we wanted to implement the one with no reply we should name it If we find a way of flushing data and ending the connection then it can be called |
While it is a hidden way, there seems to be a way to flush data to the connection before closing the Even though it is a private method, we can duplicate this code within Fiber using public-facing methods (though accessing the underlying We could use this to implement If we want to exactly mimic the ExpressJS API in that only calling What are your thoughts about this? |
@grivera64 If copying the function is the only way, seems reasonable in this case to be able to add the The functionality for |
Sounds good!
Yeah, fasthttp doesn't provide public access to the underlying writer used for flushing. So I believe the only ways to "end" a connection are to:
I could make a separate issue/PR for tracking the |
Feature Proposal Description
This issue proposes to add a method to
fiber.Ctx
that allows users to silently terminate HTTP requests in Fiber without sending any response headers nor response body. It should be a new method for theCtx
interface.This feature will be helpful for:
This feature will work by closing the underlying context's
net.Conn
using thefasthttp.(*RequestCtx).Conn().Close()
method. Fiber provides access to the underlyingfasthttp.(*RequestCtx)
throughc.Context()
. Thank you, @guno1928, for helping me find the simplest way to do this on Fiber's Discord Server! Your help is greatly appreciated.The method's name is currently undecided, but here are a few ideas:
c.End()
(look at Alignment with Express API)(Drop()
is different from the Express equivalentres.end()
)c.Close()
c.Terminate()
c.Stop()
c.Drop()
Please feel free to give to an opinion of which of these (or any other recommendations) would be a clear name for the method.
Alignment with Express API
The Express API uses the function
response.connection.end()
response.connection.destroy()
to silently terminate HTTP requests:To be similar to Express.js, this feature proposes to use a single method for the same functionality, called
Drop()
.HTTP RFC Standards Compliance
N/A, this feature will not affect how Fiber complies with the HTTP RFC standards.
API Stability
This feature will not require changing any existing methods in
fiber.Ctx
, so it should be safe to add. As fasthttp's interface is stable, there should be minimal changes or deprecations in the future.Feature Examples
Checklist:
The text was updated successfully, but these errors were encountered: