-
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
feat: add ShutdownWithContext #1383
Conversation
2bce7f6
to
1a01bcd
Compare
1a01bcd
to
d4c172d
Compare
select { | ||
case <-ctx.Done(): | ||
err = ctx.Err() | ||
break END |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering, should this then force close all connections? What does net/http
do in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering, should this then force close all connections? What does
net/http
do in this case?
The use of Shutdown
does not force all connections to close, but rather uses context to control the timeout after all listener
have been closed.
|
Perhaps call the function |
ln := fasthttputil.NewInmemoryListener() | ||
s := &Server{ | ||
Handler: func(ctx *RequestCtx) { | ||
ctx.Success("aaa/bbb", []byte("real response")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a time.Sleep(time.Millisecond*100)
here and test that ShutdownWithContext
returns correctly after the timeout? The test you have now just tests the code that is similar to what TestShutdown
already tests. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikdubbelboer PTAL
536e21c
to
88c5c84
Compare
88c5c84
to
234dd6a
Compare
Thanks! |
When will it be tagged? |
Next week I'll tag a release. |
pr for #1389
ShutdownWithContext
function to support shutdown with timeout and timeout happen after all listener already close.