fasthttp icon indicating copy to clipboard operation
fasthttp copied to clipboard

Improve RequestCtx context documentation

Open dimovnike opened this issue 3 years ago • 3 comments

The context doesnt become Done() on the timeout.

for example, this code:

func main() {
	fasthttp.ListenAndServe(":8080", fasthttp.TimeoutHandler(func(ctx *fasthttp.RequestCtx) {
		log.Println("begin")

		tm := time.NewTimer(10 * time.Second)

		select {
		case <-ctx.Done():
			log.Println("context cancelled")
			return
		case <-tm.C:
			log.Println("process finished ok")
		}

		log.Println("end")
	}, time.Second*2, "timeout"))
}

prints:

2022/02/16 12:52:56 begin
2022/02/16 12:53:06 process finished ok
2022/02/16 12:53:06 end

The client receives the timeout after 2 seconds, and the handler finishes after 10 seconds yet. It would be nice if the context became Done() in this case. If this would break existing code expectations then it can be made optional (via setting or a new TimeoutHandlerWithShutdown or something like it).

Will you accept a PR for this?

dimovnike avatar Feb 16 '22 10:02 dimovnike

note that the het/http behaves as (intuitively) expected:

func main() {
	srv := http.Server{
		Addr: ":8080",
		Handler: http.TimeoutHandler(http.HandlerFunc(
			func(w http.ResponseWriter, req *http.Request) {
				log.Println("begin")

				tm := time.NewTimer(10 * time.Second)

				select {
				case <-req.Context().Done():
					log.Println("context cancelled")
					return
				case <-tm.C:
					log.Println("process finished ok")
				}

				log.Println("end")
			},
		), 1*time.Second, "Timeout!\n"),
	}

	if err := srv.ListenAndServe(); err != nil {
		fmt.Printf("Server failed: %s\n", err)
	}
}

prints

2022/02/16 13:08:25 begin
2022/02/16 13:08:26 context cancelled

dimovnike avatar Feb 16 '22 11:02 dimovnike

We need better documentation regarding the RequestCtx implementing the Context interface. The channel returned by RequestCtx.Done() is only closed when the server is shutting down. And for performance reasons I don't think that will ever change. Creating a new channel for every request is just too expensive. If people want to use contexts they really shouldn't be using fasthttp. When you are using contexts it shows that you don't need the performance increase fasthttp gives. If you really needed that performance you wouldn't be using contexts to begin with.

erikdubbelboer avatar Feb 16 '22 14:02 erikdubbelboer

We need better documentation regarding the RequestCtx implementing the Context interface. The channel returned by RequestCtx.Done() is only closed when the server is shutting down. And for performance reasons I don't think that will ever change. Creating a new channel for every request is just too expensive. If people want to use contexts they really shouldn't be using fasthttp. When you are using contexts it shows that you don't need the performance increase fasthttp gives. If you really needed that performance you wouldn't be using contexts to begin with.

It definitely should be in the fasthttp might not be for you! section on the README. I ended up rewriting an entire project (fiber based) just because of that.

a5r0n avatar Jul 14 '22 22:07 a5r0n