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 34 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
34 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
21169dc
Merge branch 'main' into jiejaitt-feature/improve-shutdown-with-context
gaby Nov 30, 2024
be64a51
Merge branch 'main' of github.com:JIeJaitt/fiber into jiejaitt-featur…
JIeJaitt Dec 10, 2024
87b2aab
♻️ Refactor: replaced OnShutdown by OnPreShutdown and OnPostShutdown
JIeJaitt Dec 10, 2024
3389913
♻️ Refactor: streamline post-shutdown hook execution in graceful shut…
JIeJaitt Dec 11, 2024
3703600
Merge branch 'main' of github.com:JIeJaitt/fiber into jiejaitt-featur…
JIeJaitt Dec 11, 2024
6aeb048
🚨 Test: add test for gracefulShutdown
JIeJaitt Dec 11, 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 @@ -880,16 +880,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 {
Copy link
Member

Choose a reason for hiding this comment

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

Here we can define executeOnPostShutdownHooks hook with defer as well as PreShutdownHook

Copy link
Contributor Author

@JIeJaitt JIeJaitt Dec 17, 2024

Choose a reason for hiding this comment

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

Here we can define executeOnPostShutdownHooks hook with defer as well as PreShutdownHook

Is that what you mean?I think PreShutdownHook and PostShutdownHook set outside the function and then executed at ShutdownWithContext

func (app *App) ShutdownWithContext(ctx context.Context) error {
	app.mutex.Lock()
	defer app.mutex.Unlock()

	if app.server == nil {
		return ErrNotRunning
	}

	// Execute the Shutdown hook
	if app.hooks != nil {
		app.hooks.executeOnPreShutdownHooks()
	}
	defer app.hooks.executeOnPostShutdownHooks(nil)

	return app.server.ShutdownWithContext(ctx)
}

Copy link
Member

Choose a reason for hiding this comment

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

Here we can define executeOnPostShutdownHooks hook with defer as well as PreShutdownHook

Is that what you mean?I think PreShutdownHook and PostShutdownHook set outside the function and then executed at ShutdownWithContext

func (app *App) ShutdownWithContext(ctx context.Context) error {
	app.mutex.Lock()
	defer app.mutex.Unlock()

	if app.server == nil {
		return ErrNotRunning
	}

	// Execute the Shutdown hook
	if app.hooks != nil {
		app.hooks.executeOnPreShutdownHooks()
	}
	defer app.hooks.executeOnPostShutdownHooks(nil)

	return app.server.ShutdownWithContext(ctx)
}

You can do it like:

func (app *App) ShutdownWithContext(ctx context.Context) error {
	app.mutex.Lock()
	defer app.mutex.Unlock()

	var err error

	if app.server == nil {
		return ErrNotRunning
	}

	// Execute the Shutdown hook
	app.hooks.executeOnPreShutdownHooks()
	defer app.hooks.executeOnPostShutdownHooks(err)

	err = app.server.ShutdownWithContext(ctx)
	return err
}

So that, we won't need to add another additional check to listen.go and this is going to work even if you use app.Shutdown() separately. However, this hook wouldn't work if app.Listen() hadn't been started in another goroutine. Maybe we can also add a note to indicate it -> https://github.com/valyala/fasthttp/blob/master/server.go#L1830

// 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 @@ -21,6 +21,7 @@ import (
"regexp"
"runtime"
"strings"
"sync/atomic"
"testing"
"time"

Expand Down Expand Up @@ -861,45 +862,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)
efectn marked this conversation as resolved.
Show resolved Hide resolved

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.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this behavior. I think we should only execute them during successful shutdown. Maybe we can OnShutdownError hook. What do you think @gaby @ReneWerner87

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure either. Also not sure about the defer of the hook

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to add Post Pre hook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The add Post Pre hook is a good idea, but how do I tell which hooks are pre closed and which are post closed? @efectn

Copy link
Member

Choose a reason for hiding this comment

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

You can add new hook called PostShutdown, PreShutdown and replace the old one with them

Copy link
Member

Choose a reason for hiding this comment

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

sound good, @JIeJaitt can you do the suggested change

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 means making a change below the type Hooks struct to add these two fields that mean PreShutdownHandler and PostShutdownHandler respectively , if I understand it correctly I'll try to implement it @ReneWerner87 @efectn @gaby

Copy link
Member

Choose a reason for hiding this comment

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

This means making a change below the type Hooks struct to add these two fields that mean PreShutdownHandler and PostShutdownHandler respectively , if I understand it correctly I'll try to implement it @ReneWerner87 @efectn @gaby

Yes it is. You can also add err parameter to PostShutdown hook, so that we can also get rid of OnShutdownError, OnShutdownSuccess properties of ListenerConfig.

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 means making a change below the type Hooks struct to add these two fields that mean PreShutdownHandler and PostShutdownHandler respectively , if I understand it correctly I'll try to implement it @ReneWerner87 @efectn @gaby

Yes it is. You can also add err parameter to PostShutdown hook, so that we can also get rid of OnShutdownError, OnShutdownSuccess properties of ListenerConfig.

OK, I will press on with this idea to realize it again in my free time, by the way what is the v3 deadline as I am busy with my final exams these days.

Copy link
Member

Choose a reason for hiding this comment

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

This means making a change below the type Hooks struct to add these two fields that mean PreShutdownHandler and PostShutdownHandler respectively , if I understand it correctly I'll try to implement it @ReneWerner87 @efectn @gaby

Yes it is. You can also add err parameter to PostShutdown hook, so that we can also get rid of OnShutdownError, OnShutdownSuccess properties of ListenerConfig.

OK, I will press on with this idea to realize it again in my free time, by the way what is the v3 deadline as I am busy with my final exams these days.

We want to release v3 rc1 in the beginning of January. If you can't look at the PR this week, i can also look no problem


```go
func (app *App) Shutdown() error
Expand Down
Loading