Skip to content
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

TimeoutHandler new goroutine for each request? #565

Closed
a-urth opened this issue Apr 24, 2019 · 5 comments
Closed

TimeoutHandler new goroutine for each request? #565

a-urth opened this issue Apr 24, 2019 · 5 comments

Comments

@a-urth
Copy link

a-urth commented Apr 24, 2019

I've noticed a lot of goroutines created while tracing our application, which uses fasthttp. And those were created here:

fasthttp/server.go

Lines 393 to 397 in ed16dc0

go func() {
h(ctx)
ch <- struct{}{}
<-concurrencyCh
}()

I understand why its like that, so my question is rather - doesn't it contradict idea of fasthttp processing requests in workers pool and making as less allocations as possible?

@erikdubbelboer
Copy link
Collaborator

We should indeed use a worker pool here as well.

I guess nobody using fasthttp for high qps workloads uses the TimeoutHandler. I know I don't cause I prefer to handle timeouts in my own code so I can abandon the work instead of just returning the response early but still continuing to do the processing.

@a-urth
Copy link
Author

a-urth commented Apr 24, 2019

For us its the same, timeout was used... just because i guess. But I noticed that and was curious why its like that.
Anyway, thanks for the answer 👍

@ms2008
Copy link

ms2008 commented Jun 4, 2019

Go1.11 has many optimizations for stack copy. After upgrading to Go1.1, is it necessary to use goroutine pool any more?

Had been discussed here:

pingcap/tidb#7564

@erikdubbelboer
Copy link
Collaborator

@ms2008 using a goroutine pool or not has nothing to do with this issue. But the question if we should abandon the worker pool we are using now in fasthttp is an interesting one. I wasn't aware of these optimizations. It's something I'm going to investigate in the near future when I have some time.

@erikdubbelboer
Copy link
Collaborator

I have done a lot of testing and having a goroutine pool is still slightly faster than just starting a new goroutine every time you need one.

Seeing as our code is currently stable with the pool I don't think we should remove it at this point. But maybe we should in the future to simplify our code.

Since the difference is really small I don't think we should complicate our code and implement a pool for TimeoutHandler. Especially if we might want to remove our goroutine pool in the future.

So I'm closing this issue for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants