-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
gracefully shuts down bug. #958
Comments
Good idea. I think here: Line 2199 in 5661df8
It should include || atomic.LoadInt32(&s.stop) == 1
Can you test this and make a pull request? Thanks. |
hey @alpha-baby, thanks for your report. I'm concerned this one could introduce bugs in case of fasthttp service being behind ALB, 'cause the client would close the connection to ALB, then open it again unnecessarily, which will cause a spike in load during rollouts. I'd prefer having this under an option |
@kirillDanshin good point I didn't think of that. @alpha-baby can you add an option to the |
Ok,tomorrow i will commit new code. |
before i commit this PR, tcp connection also will be close when you call Closing connection source code in here: https://github.com/valyala/fasthttp/blob/master/server.go#L2303 |
The connections being closed is what is supposed to happen. What people might not want is the connection from the client to the loadbalancer being terminated as well if there is a loadbalancer. |
* fix gracefilly shutdown bug, issue #958 * fix golangci-lint * add option: CloseOnShutdown into Sever * Update server.go Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com> * Update server.go Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com> Co-authored-by: fujianhao3 <fujianhao3@jd.com> Co-authored-by: Erik Dubbelboer <erik@dubbelboer.com>
when invocate
func (s *Server) Shutdown() error
, should return header:Connection: close
if you sure it's a bug, i will commit a PR to fix it.
The text was updated successfully, but these errors were encountered: