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

🔥 feat: Improve and Optimize ShutdownWithContext Func #3162

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
137da86
feat: Optimize ShutdownWithContext method in app.go
Oct 10, 2024
b41d084
feat: Enhance ShutdownWithContext test for improved reliability
Oct 10, 2024
e1ad8e9
Merge branch 'gofiber:main' into jiejaitt-feature/improve-shutdown-wi…
JIeJaitt Oct 12, 2024
a683166
📚 Doc: update the docs to explain shutdown & hook execution order
JIeJaitt Oct 12, 2024
a4a5831
Merge branch 'main' into jiejaitt-feature/improve-shutdown-with-context
JIeJaitt Oct 12, 2024
424ecbb
Merge branch 'main' of github.com:JIeJaitt/fiber into jiejaitt-featur…
JIeJaitt Oct 12, 2024
98fbdff
Merge branch 'jiejaitt-feature/improve-shutdown-with-context' of gith…
JIeJaitt Oct 12, 2024
796922f
🩹 Fix: Possible Data Race on shutdownHookCalled Variable
JIeJaitt Oct 12, 2024
e465b5b
🩹 Fix: Remove the default Case
JIeJaitt Oct 12, 2024
ee866ec
🩹 Fix: Import sync/atomic
JIeJaitt Oct 12, 2024
e0a56be
🩹 Fix: golangci-lint problem
JIeJaitt Oct 13, 2024
d01a09e
Merge branches 'jiejaitt-feature/improve-shutdown-with-context' and '…
JIeJaitt Oct 13, 2024
750a7fa
🎨 Style: add block in api.md
JIeJaitt Oct 28, 2024
2ea0bb3
Merge branch 'main' of github.com:JIeJaitt/fiber into jiejaitt-featur…
JIeJaitt Oct 28, 2024
b9509fe
🩹 Fix: go mod tidy
JIeJaitt Oct 28, 2024
c2792e7
feat: Optimize ShutdownWithContext method in app.go
Oct 10, 2024
18111e5
feat: Enhance ShutdownWithContext test for improved reliability
Oct 10, 2024
83ea43d
📚 Doc: update the docs to explain shutdown & hook execution order
JIeJaitt Oct 12, 2024
66dcb42
🩹 Fix: Possible Data Race on shutdownHookCalled Variable
JIeJaitt Oct 12, 2024
f3902c5
🩹 Fix: Remove the default Case
JIeJaitt Oct 12, 2024
da193ac
🩹 Fix: Import sync/atomic
JIeJaitt Oct 12, 2024
0a92125
🩹 Fix: golangci-lint problem
JIeJaitt Oct 13, 2024
b0bc70c
🎨 Style: add block in api.md
JIeJaitt Oct 28, 2024
44cbc62
🩹 Fix: go mod tidy
JIeJaitt Oct 28, 2024
0e99032
Merge branch 'jiejaitt-feature/improve-shutdown-with-context' of gith…
JIeJaitt Oct 28, 2024
a32bddd
Merge branch 'main' of github.com:gofiber/fiber into jiejaitt-feature…
JIeJaitt Oct 29, 2024
5df1c89
Merge branch 'main' of github.com:gofiber/fiber into jiejaitt-feature…
JIeJaitt Nov 14, 2024
da8c54d
Merge branch 'main' of github.com:gofiber/fiber into jiejaitt-feature…
JIeJaitt Nov 15, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions app.go
Original file line number Diff line number Diff line change
Expand Up @@ -841,16 +841,18 @@ func (app *App) ShutdownWithTimeout(timeout time.Duration) error {
//
// ShutdownWithContext does not close keepalive connections so its recommended to set ReadTimeout to something else than 0.
func (app *App) ShutdownWithContext(ctx context.Context) error {
if app.hooks != nil {
// TODO: check should be defered?
app.hooks.executeOnShutdownHooks()
}

app.mutex.Lock()
defer app.mutex.Unlock()

if app.server == nil {
return ErrNotRunning
}

// Execute shutdown hooks in a deferred function
if app.hooks != nil {
defer app.hooks.executeOnShutdownHooks()
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
}

return app.server.ShutdownWithContext(ctx)
}

Expand Down
40 changes: 28 additions & 12 deletions app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"regexp"
"runtime"
"strings"
"sync/atomic"
"testing"
"time"

Expand Down Expand Up @@ -860,45 +861,60 @@ func Test_App_ShutdownWithContext(t *testing.T) {
t.Parallel()

app := New()
var shutdownHookCalled atomic.Int32
app.Hooks().OnShutdown(func() error {
shutdownHookCalled.Store(1)
return nil
})

app.Get("/", func(ctx Ctx) error {
time.Sleep(5 * time.Second)
return ctx.SendString("body")
})

ln := fasthttputil.NewInmemoryListener()

serverErr := make(chan error, 1)
go func() {
err := app.Listener(ln)
assert.NoError(t, err)
serverErr <- app.Listener(ln)
}()

time.Sleep(1 * time.Second)
time.Sleep(100 * time.Millisecond)

clientDone := make(chan struct{})
go func() {
conn, err := ln.Dial()
assert.NoError(t, err)

_, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: google.com\r\n\r\n"))
_, err = conn.Write([]byte("GET / HTTP/1.1\r\nHost: example.com\r\n\r\n"))
assert.NoError(t, err)
close(clientDone)
}()

time.Sleep(1 * time.Second)
<-clientDone
// Sleep to ensure the server has started processing the request
time.Sleep(100 * time.Millisecond)
Comment on lines +893 to +895
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Suggestion: Replace sleep with synchronization for test reliability

Using time.Sleep(100 * time.Millisecond) to wait for the server to start processing the request might lead to flaky tests. Consider using synchronization primitives like channels or sync.WaitGroup to ensure the server has begun processing before proceeding.


shutdownErr := make(chan error)
shutdownErr := make(chan error, 1)
go func() {
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
defer cancel()
shutdownErr <- app.ShutdownWithContext(ctx)
}()

select {
case <-time.After(5 * time.Second):
t.Fatal("idle connections not closed on shutdown")
case <-time.After(2 * time.Second):
t.Fatal("shutdown did not complete in time")
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved
case err := <-shutdownErr:
if err == nil || !errors.Is(err, context.DeadlineExceeded) {
t.Fatalf("unexpected err %v. Expecting %v", err, context.DeadlineExceeded)
}
require.Error(t, err, "Expected shutdown to return an error due to timeout")
require.ErrorIs(t, err, context.DeadlineExceeded, "Expected DeadlineExceeded error")
}

assert.Equal(t, int32(1), shutdownHookCalled.Load(), "Shutdown hook was not called")

err := <-serverErr
assert.NoError(t, err, "Server should have shut down without error")
// default:
// Server is still running, which is expected as the long-running request prevented full shutdown
}

// go test -run Test_App_Mixed_Routes_WithSameLen
Expand Down
2 changes: 1 addition & 1 deletion docs/api/fiber.md
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ Shutdown gracefully shuts down the server without interrupting any active connec

ShutdownWithTimeout will forcefully close any active connections after the timeout expires.

ShutdownWithContext shuts down the server including by force if the context's deadline is exceeded.
ShutdownWithContext shuts down the server including by force if the context's deadline is exceeded.Shutdown hooks will still be executed, even if an error occurs during the shutdown process, as they are deferred to ensure cleanup happens regardless of errors.
ReneWerner87 marked this conversation as resolved.
Show resolved Hide resolved

```go
func (app *App) Shutdown() error
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ require (
github.com/philhofer/fwd v1.1.3-0.20240612014219-fbbf4953d986 // indirect
github.com/pmezard/go-difflib v1.0.0 // indirect
github.com/valyala/tcplisten v1.0.0 // indirect
golang.org/dl v0.0.0-20241001165935-bedb0f791d00 // indirect
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an official multi-version control function of the go language, because the Go version I use locally is higher than the go version in fiber's go mod. In order to keep the version consistent, I rely on this method to use it locally in the same way as the fiber framework. Go language version, you can see this to get more information: https://pkg.go.dev/golang.org/dl and https://pkg.go.dev/golang.org/[email protected]/go1.22.6 . It will not have any impact on the function of the fiber

golang.org/x/net v0.29.0 // indirect
golang.org/x/sys v0.25.0 // indirect
golang.org/x/text v0.18.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ github.com/valyala/fasthttp v1.56.0 h1:bEZdJev/6LCBlpdORfrLu/WOZXXxvrUQSiyniuaoW
github.com/valyala/fasthttp v1.56.0/go.mod h1:sReBt3XZVnudxuLOx4J/fMrJVorWRiWY2koQKgABiVI=
github.com/valyala/tcplisten v1.0.0 h1:rBHj/Xf+E1tRGZyWIWwJDiRY0zc1Js+CV5DqwacVSA8=
github.com/valyala/tcplisten v1.0.0/go.mod h1:T0xQ8SeCZGxckz9qRXTfG43PvQ/mcWh7FwZEA7Ioqkc=
golang.org/dl v0.0.0-20241001165935-bedb0f791d00 h1:OX0WPBB1pQPZy1SL0+q5C/VuuM6e1wv6uEuB9iyBi/I=
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as my answer above

golang.org/dl v0.0.0-20241001165935-bedb0f791d00/go.mod h1:fwQ+hlTD8I6TIzOGkQqxQNfE2xqR+y7SzGaDkksVFkw=
golang.org/x/net v0.29.0 h1:5ORfpBpCs4HzDYoodCDBbwHzdR5UrLBZ3sOnUJmFoHo=
golang.org/x/net v0.29.0/go.mod h1:gLkgy8jTGERgjzMic6DS9+SP0ajcu6Xu3Orq/SpETg0=
golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
Expand Down
Loading