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

Conversation

JIeJaitt
Copy link
Contributor

@JIeJaitt JIeJaitt commented Oct 10, 2024

Description

Optimize ShutdownWithContext function and elegant shutdown flow of fiber, streamline Listen structure

Changes introduced

  • By holding the mutex lock throughout the operation, any modifications to the app's state are protected. defer ensures that the hook function is executed only after it is unlocked. At this time, no other goroutine will access the shared resource at the same time, ensuring concurrency safety.
  • defer ensures that the hook function will be executed regardless of whether an error occurs in the function. It also avoids execution order issues and further ensures concurrency safety.
  • Using executeOnPreShutdownHooks and executeOnPostShutdownHooks Instead of OnShutdownSuccess and OnShutdownError

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • Conducted a self-review of the code and provided comments for complex or critical parts.
  • Updated the documentation in the /docs/ directory for Fiber's documentation.
  • Added or updated unit tests to validate the effectiveness of the changes or new features.
  • Ensured that new and existing unit tests pass locally with the changes.
  • Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

yingjie.huang added 2 commits October 10, 2024 16:44
- Reorder mutex lock acquisition to the start of the function
- Early return if server is not running
- Use defer for executing shutdown hooks
- Simplify nil check for hooks
- Remove TODO comment

This commit improves the readability, robustness, and execution order
of the shutdown process. It ensures consistent state throughout the
shutdown and guarantees hook execution even in error cases.
- Add shutdown hook verification
- Implement better synchronization with channels
- Improve error handling and assertions
- Adjust timeouts for more consistent results
- Add server state check after shutdown attempt
- Include comments explaining expected behavior

This commit improves the comprehensiveness and reliability of the
ShutdownWithContext test, ensuring proper verification of shutdown
hooks, timeout behavior, and server state during long-running requests.
Copy link

welcome bot commented Oct 10, 2024

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@JIeJaitt JIeJaitt changed the title Jiejaitt feature/improve shutdown with context Feature: Complete TODO and Optimize ShutdownWithContext Func Oct 10, 2024
@gaby gaby added v3 and removed ✏️ Feature labels Oct 10, 2024
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.14%. Comparing base (283ef32) to head (cda210b).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
hooks.go 91.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3162      +/-   ##
==========================================
- Coverage   84.15%   84.14%   -0.01%     
==========================================
  Files         116      116              
  Lines       11551    11558       +7     
==========================================
+ Hits         9721     9726       +5     
- Misses       1399     1401       +2     
  Partials      431      431              
Flag Coverage Δ
unittests 84.14% <94.11%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

app_test.go Outdated
}()

<-clientDone
time.Sleep(100 * time.Millisecond)
Copy link
Member

Choose a reason for hiding this comment

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

why do we need a sleep here? i think it's arbitrary since we already wait for clientDone channel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we need a sleep here? i think it's arbitrary since we already wait for clientDone channel

This sleep is to ensure that our server has started processing the request. Although we have confirmed that the client has sent the request through <-clientDone, this means that the request has been sent to the server's listening port, and there is no guarantee that the server's processing coroutine has been sent. Start processing the request, so I think sleep 100 * Millisecond is required here as well.

@JIeJaitt JIeJaitt marked this pull request as ready for review October 12, 2024 15:32
@JIeJaitt JIeJaitt requested a review from a team as a code owner October 12, 2024 15:32
@JIeJaitt JIeJaitt requested review from gaby, sixcolors, ReneWerner87 and efectn and removed request for a team October 12, 2024 15:32
Copy link
Contributor

coderabbitai bot commented Oct 12, 2024

Warning

Rate limit exceeded

@JIeJaitt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 28 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 05e14f5 and cda210b.

📒 Files selected for processing (1)
  • docs/api/hooks.md (3 hunks)

Walkthrough

The changes enhance the Fiber web framework by introducing a new method New for creating an instance of App with customizable configuration options. The Listen method now accepts an optional ListenConfig parameter for server customization. Additionally, the ShutdownWithContext method has been updated to clarify the execution of shutdown hooks and improve control flow during shutdown. Documentation has been expanded to provide detailed descriptions and examples for these new features, ensuring clarity for users.

Changes

File(s) Change Summary
docs/api/fiber.md Updated documentation to include new New method, detailed Config structure, and examples for app instantiation and server listening.
app.go Updated ShutdownWithContext method to check server state before shutdown and reorder hook execution.
app_test.go Enhanced tests for ShutdownWithContext, added atomic operations for shutdown hook verification, and refined error handling during shutdown.
hooks.go Restructured shutdown hook handling by introducing OnPreShutdownHandler and OnPostShutdownHandler, replacing the previous OnShutdownHandler. Updated execution methods for new hooks.
hooks_test.go Renamed Test_Hook_OnShutdown to Test_Hook_OnPreShutdown, added Test_Hook_OnPostShutdown, and updated assertions for shutdown hooks.
listen.go Removed OnShutdownError and OnShutdownSuccess fields from ListenConfig, centralizing error handling in hooks.
listen_test.go Restructured Test_Listen_Graceful_Shutdown into a new helper function testGracefulShutdown for better organization and clarity.
docs/api/hooks.md Introduced OnPreShutdown and OnPostShutdown hooks, updated related documentation to reflect the changes.
docs/whats_new.md Added a new section detailing significant updates in version 3, including new hooks and method changes.

Possibly related PRs

  • 🩹 Fix: goroutine leakage #3306: The changes in the main PR regarding the ShutdownWithContext method in app.go are related to the modifications in the same method in the retrieved PR, which also involves error handling and control flow during shutdown processes.
  • 🔥 feat: Add support for graceful shutdown timeout in ListenConfig #3220: The changes in the main PR regarding the ShutdownWithContext method's error handling and execution order are directly related to the modifications in the retrieved PR, which also focuses on enhancing shutdown processes, including the introduction of a ShutdownTimeout feature.

Suggested reviewers

  • sixcolors
  • gaby
  • ReneWerner87
  • efectn

Poem

In the garden of code, changes bloom bright,
With hooks that now dance, in the soft moonlight.
Shutdowns are smoother, no more race to the end,
Documentation's richer, our framework's new friend.
So hop with delight, let the updates take flight! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@gaby gaby changed the title Feature: Complete TODO and Optimize ShutdownWithContext Func feat: Complete TODO and Optimize ShutdownWithContext Func Oct 12, 2024
@gaby gaby changed the title feat: Complete TODO and Optimize ShutdownWithContext Func feat: Improve and Optimize ShutdownWithContext Func Oct 12, 2024
…ub.com:JIeJaitt/fiber into jiejaitt-feature/improve-shutdown-with-context
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
docs/api/fiber.md (1)

208-208: Approve with minor suggestions for improvement

The added description for ShutdownWithContext is informative and aligns well with the PR objectives. It provides important details about the method's behavior, especially regarding the execution of shutdown hooks. To enhance clarity and readability, consider the following suggestions:

  1. Add a comma after "including" for better sentence structure.
  2. Split the long sentence into two for improved readability.

Here's a suggested revision:

- 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.
+ 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.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~208-~208: Possible missing comma found.
Context: ...es. ShutdownWithContext shuts down the server including by force if the context's dea...

(AI_HYDRA_LEO_MISSING_COMMA)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9dd3d94 and 98fbdff.

📒 Files selected for processing (3)
  • app.go (1 hunks)
  • app_test.go (1 hunks)
  • docs/api/fiber.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/api/fiber.md

[uncategorized] ~208-~208: Possible missing comma found.
Context: ...es. ShutdownWithContext shuts down the server including by force if the context's dea...

(AI_HYDRA_LEO_MISSING_COMMA)

🔇 Additional comments (1)
app.go (1)

846-855: Improved shutdown process with deferred hook execution

The changes to the ShutdownWithContext method enhance the shutdown process in several ways:

  1. Thread-safety is ensured by acquiring a mutex lock at the beginning of the method.
  2. A nil check for the server prevents potential panics if the server hasn't been initialized.
  3. The deferral of executeOnShutdownHooks guarantees that shutdown hooks are executed even if an error occurs during the shutdown process.

These improvements contribute to a more robust and reliable shutdown mechanism.

app_test.go Outdated
Comment on lines 897 to 900
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
defer cancel()
shutdownErr <- app.ShutdownWithContext(ctx)
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure Proper Context Cancellation

The context created with context.WithTimeout should have its cancellation function deferred immediately to prevent potential resource leaks, even if an error occurs before the defer statement is reached.

Suggested Improvement

Move the defer cancel() directly after context creation.

	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+	defer cancel()
	shutdownErr <- app.ShutdownWithContext(ctx)
-	defer cancel()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
defer cancel()
shutdownErr <- app.ShutdownWithContext(ctx)
}()
ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
defer cancel()
shutdownErr <- app.ShutdownWithContext(ctx)
}()

app_test.go Outdated
Comment on lines 913 to 917
case 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
}
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

⚠️ Potential issue

Avoid Skipping Server Error Handling

Using a default case in the select statement may cause the test to proceed without waiting for the server to shut down, potentially leading to flaky tests. It's important to handle the server error explicitly.

Suggested Fix: Remove the default Case

This ensures the test waits for serverErr to receive an error or completes the shutdown process.

	select {
	case 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
	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
case 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
}
case err := <-serverErr:
assert.NoError(t, err, "Server should have shut down without error")

app_test.go Outdated
Comment on lines 863 to 867
shutdownHookCalled := false
app.Hooks().OnShutdown(func() error {
shutdownHookCalled = true
return nil
})
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Possible Data Race on shutdownHookCalled Variable

The shutdownHookCalled boolean is accessed by multiple goroutines without proper synchronization, which could lead to a data race. In Go, variables shared between goroutines should be protected using synchronization mechanisms like channels, mutexes, or atomic operations.

Suggested Fix: Use an Atomic Variable

Replace shutdownHookCalled with an int32 and use atomic.StoreInt32 and atomic.LoadInt32 for thread-safe operations.

+import "sync/atomic"

-	shutdownHookCalled := false
+	var shutdownHookCalled int32

	app.Hooks().OnShutdown(func() error {
-		shutdownHookCalled = true
+		atomic.StoreInt32(&shutdownHookCalled, 1)
		return nil
	})

	// ...

-	assert.True(t, shutdownHookCalled, "Shutdown hook was not called")
+	assert.Equal(t, int32(1), atomic.LoadInt32(&shutdownHookCalled), "Shutdown hook was not called")

Committable suggestion was skipped due to low confidence.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (3)
hooks_test.go (1)

202-248: Improve synchronization in error propagation test.

While the test effectively validates error propagation, consider enhancing the synchronization:

  1. Add a timeout to channel operations to prevent potential deadlocks
  2. Use a WaitGroup to ensure proper server shutdown before test completion
 t.Run("should execute post shutdown hook with error", func(t *testing.T) {
     app := New()
     expectedErr := errors.New("test shutdown error")
+    var wg sync.WaitGroup
+    wg.Add(1)

     // Use channels to synchronize and pass results
     hookResult := make(chan struct {
         err    error
         called bool
-    }, 1)
+    }, 1) // Buffer size of 1 prevents deadlock
+    defer close(hookResult)

     app.Hooks().OnPostShutdown(func(err error) error {
         hookResult <- struct {
             err    error
             called bool
         }{
             err:    err,
             called: true,
         }
         return nil
     })

     // Use channel to make sure the server is up
     serverReady := make(chan struct{})
+    defer close(serverReady)

     go func() {
+        defer wg.Done()
         serverReady <- struct{}{} // Signal that the server is ready to start
         if err := app.Listen(":0"); err != nil {
             t.Errorf("Failed to start listener: %v", err)
         }
     }()

     select {
     case <-serverReady: // Wait for the server to be ready
+    case <-time.After(5 * time.Second):
+        t.Fatal("Server startup timeout")
     }

     app.hooks.executeOnPostShutdownHooks(expectedErr)

     // Wait for the hook to finish executing and get the result
-    result := <-hookResult
+    var result struct {
+        err    error
+        called bool
+    }
+    select {
+    case result = <-hookResult:
+    case <-time.After(5 * time.Second):
+        t.Fatal("Hook execution timeout")
+    }

     if !result.called {
         t.Fatal("hook was not called")
     }

     if !errors.Is(result.err, expectedErr) {
         t.Fatalf("hook received wrong error: want %v, got %v", expectedErr, result.err)
     }
+    wg.Wait() // Ensure server goroutine completes
 })
app_test.go (1)

993-994: Replace sleep with proper synchronization.

Using time.Sleep for test synchronization can lead to flaky tests. Consider using a channel to signal when the request has started processing.

+    requestStarted := make(chan struct{})
     app.Get("/", func(c Ctx) error {
+        close(requestStarted)
         time.Sleep(2 * time.Second)
         return c.SendString("OK")
     })

     // ... connection setup ...

-    time.Sleep(100 * time.Millisecond) // Wait for request to start
+    select {
+    case <-requestStarted:
+        // Request has started processing
+    case <-time.After(time.Second):
+        t.Fatal("Request did not start in time")
+    }
docs/api/hooks.md (1)

190-204: Add usage examples for the new shutdown hooks.

The documentation would benefit from examples showing how to use OnPreShutdown and OnPostShutdown hooks, similar to other hook examples in the document.

// Example for OnPreShutdown
app.Hooks().OnPreShutdown(func() error {
    log.Println("Preparing for shutdown...")
    return nil
})

// Example for OnPostShutdown
app.Hooks().OnPostShutdown(func(err error) error {
    if err != nil {
        log.Printf("Shutdown completed with error: %v\n", err)
    } else {
        log.Println("Shutdown completed successfully")
    }
    return nil
})
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 827d70c and 7c01623.

📒 Files selected for processing (4)
  • app_test.go (3 hunks)
  • docs/api/hooks.md (3 hunks)
  • docs/whats_new.md (2 hunks)
  • hooks_test.go (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
hooks_test.go (1)
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
app_test.go (1)
Learnt from: efectn
PR: gofiber/fiber#3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.
🪛 LanguageTool
docs/whats_new.md

[grammar] ~170-~170: It appears that hyphens are missing.
Context: ...ecated OnShutdown in favor of the new pre/post shutdown hooks - Improved shutdown hook executio...

(PRE_AND_POST_NN)

⏰ Context from checks skipped due to timeout of 90000ms (5)
  • GitHub Check: unit (1.23.x, macos-latest)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: repeated
  • GitHub Check: Compare
  • GitHub Check: Analyse
🔇 Additional comments (7)
hooks_test.go (3)

184-200: LGTM! Well-structured test for pre-shutdown hook.

The test effectively validates the pre-shutdown hook execution and cleanup. Good use of bytebufferpool for efficient memory management.


250-274: LGTM! Well-structured test for hook execution order.

The test effectively validates that multiple hooks are executed in the correct order.


276-287: LGTM! Good error handling test.

The test properly verifies that hook errors are handled gracefully without causing panics.

app_test.go (3)

849-858: Well-implemented server startup synchronization!

The use of a channel to coordinate server startup is a good practice that helps avoid race conditions and ensures proper test timing.


892-925: Well-structured test with proper error handling and cleanup!

The test case effectively validates the basic shutdown functionality with:

  • Proper context handling and cleanup
  • Comprehensive error checks
  • Clear request/response flow

927-961: Excellent hook execution order test with proper thread safety!

The test effectively validates the shutdown hook functionality:

  • Uses mutex to ensure thread-safe access to shared state
  • Verifies correct execution order of pre/post shutdown hooks
  • Includes proper error handling
docs/whats_new.md (1)

163-185: Excellent migration guide with clear examples!

The documentation effectively explains the changes to the shutdown hooks system with:

  • Clear explanation of new features
  • Helpful code examples showing before/after changes
  • Important usage notes about goroutines
🧰 Tools
🪛 LanguageTool

[grammar] ~170-~170: It appears that hyphens are missing.
Context: ...ecated OnShutdown in favor of the new pre/post shutdown hooks - Improved shutdown hook executio...

(PRE_AND_POST_NN)

@JIeJaitt
Copy link
Contributor Author

@ReneWerner87 @efectn @gaby I've updated what_new.md and hooks.md, but I'm now experiencing a large number of failed Github Action tests, it appears to be a performance issue, the code is something I don't have much of a clue about at the moment.

@ReneWerner87
Copy link
Member

we will have a look

@JIeJaitt
Copy link
Contributor Author

we will have a look

very thankful

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
hooks_test.go (1)

239-239: 🛠️ Refactor suggestion

Replace sleep with proper synchronization.

Using time.Sleep for synchronization is unreliable and can lead to flaky tests. Consider using a channel to signal when the server is ready.

-		// Wait a bit for the server to start
-		time.Sleep(100 * time.Millisecond)
+		// Use channel to signal server readiness
+		ready := make(chan struct{})
+		go func() {
+			defer close(ready)
+			if err := app.Listen(":0"); err != nil {
+				if !errors.Is(err, errors.New("server closed")) {
+					t.Errorf("unexpected error: %v", err)
+				}
+			}
+		}()
+		<-ready
🧹 Nitpick comments (1)
hooks_test.go (1)

232-236: Improve error handling in Listen.

The error handling for app.Listen can be simplified using testify's require package.

-			if err := app.Listen(":0"); err != nil {
-				if !errors.Is(err, errors.New("server closed")) {
-					t.Errorf("unexpected error: %v", err)
-				}
-			}
+			err := app.Listen(":0")
+			require.True(t, err == nil || errors.Is(err, errors.New("server closed")), "unexpected error: %v", err)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ebdccd0 and 0860595.

📒 Files selected for processing (1)
  • hooks_test.go (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
hooks_test.go (1)
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: unit (1.23.x, macos-13)
  • GitHub Check: unit (1.23.x, macos-latest)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: unit (1.23.x, ubuntu-latest)
  • GitHub Check: repeated
  • GitHub Check: Analyse
  • GitHub Check: Compare
🔇 Additional comments (1)
hooks_test.go (1)

185-201: LGTM! Clear and focused test for pre-shutdown hook.

The test case effectively validates the pre-shutdown hook execution and maintains a clear, single responsibility.

@JIeJaitt
Copy link
Contributor Author

@ReneWerner87 @efectn @gaby all github action check succeed. Please review the latest code

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
docs/api/fiber.md (1)

231-231: Add a comma for better readability.

Add a comma after "server" to improve readability.

Apply this diff:

-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.
🧰 Tools
🪛 LanguageTool

[uncategorized] ~231-~231: Possible missing comma found.
Context: ...es. ShutdownWithContext shuts down the server including by force if the context's dea...

(AI_HYDRA_LEO_MISSING_COMMA)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d5fa4e3 and d8434e0.

📒 Files selected for processing (2)
  • docs/api/fiber.md (2 hunks)
  • listen.go (2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
listen.go

[warning] 500-500: listen.go#L500
Added line #L500 was not covered by tests

🪛 LanguageTool
docs/api/fiber.md

[uncategorized] ~231-~231: Possible missing comma found.
Context: ...es. ShutdownWithContext shuts down the server including by force if the context's dea...

(AI_HYDRA_LEO_MISSING_COMMA)

⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: repeated
  • GitHub Check: Compare
🔇 Additional comments (2)
listen.go (1)

94-94: LGTM!

The documentation update correctly reflects the new hook-based error handling approach.

docs/api/fiber.md (1)

114-114: LGTM!

The documentation for ShutdownTimeout is clear, comprehensive, and accurately describes the timeout behavior and its impact on hooks.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
docs/api/hooks.md (1)

180-186: Clarify Deprecation of OnShutdown Hook
The section begins by noting that "Since v3, the OnShutdown hook is split into OnPreShutdown and OnPostShutdown." However, the documentation still includes a signature for OnShutdown (i.e. func (h *Hooks) OnShutdown(handler ...OnShutdownHandler)). To avoid potential confusion, consider marking this hook as deprecated or removing it entirely if it is no longer supported by the framework.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d8434e0 and 3cb0c12.

📒 Files selected for processing (1)
  • docs/api/hooks.md (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: unit (1.23.x, macos-13)
  • GitHub Check: unit (1.23.x, windows-latest)
  • GitHub Check: unit (1.23.x, ubuntu-latest)
  • GitHub Check: repeated
  • GitHub Check: Compare
  • GitHub Check: Analyse
🔇 Additional comments (4)
docs/api/hooks.md (4)

19-20: New Hooks Listed: OnPreShutdown and OnPostShutdown
The navigation list now includes the two new hooks. Please verify that the link anchors (#onpreshutdown and #onpostshutdown) correctly match the corresponding section headings later in the document.


33-34: New Handler Type Declarations
The new type declarations for OnPreShutdownHandler and OnPostShutdownHandler are well-defined. The signature for OnPreShutdownHandler (no parameters, returns an error) and for OnPostShutdownHandler (accepting an error and returning an error) clearly convey their intended use.


190-194: Documentation for OnPreShutdown Hook
The new OnPreShutdown section clearly documents its purpose and provides an accurate code signature. This enhancement helps users understand how to register pre-shutdown hooks that execute before the shutdown process begins.


195-199: Documentation for OnPostShutdown Hook
The OnPostShutdown documentation is clearly presented, indicating that the hook is executed after shutdown and is intended for error handling. This addition improves the overall clarity of the shutdown hook mechanism and guides users on how to implement post-shutdown logic.

Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

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

LGTM, just a small change in the docs

@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 20, 2025

@JIeJaitt last two doc changes and then i will merge

@JIeJaitt JIeJaitt requested a review from gaby February 22, 2025 16:06
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (5)
listen_test.go (3)

92-94: Improve error handling in connection close.

Consider wrapping the error with additional context to aid debugging.

-if err := conn.Close(); err != nil {
-    t.Logf("error closing connection: %v", err)
-}
+if err := conn.Close(); err != nil {
+    t.Logf("error closing test connection during readiness check: %v", err)
+}

113-130: Consider adding more test coverage.

While the current test cases cover basic scenarios, consider adding edge cases such as:

  • Server shutdown during an active request
  • Multiple concurrent requests during shutdown
  • Very large response bodies

157-160: Add comments explaining timeout behavior.

The nanosecond timeout test case could benefit from comments explaining why this specific timeout value was chosen and what behavior it's testing.

+// Using 1 nanosecond timeout to force a context.DeadlineExceeded error,
+// verifying that the shutdown process respects the timeout configuration
 if shutdownTimeout == 1*time.Nanosecond {
     require.Error(t, receivedErr)
     require.ErrorIs(t, receivedErr, context.DeadlineExceeded)
 }
docs/api/hooks.md (2)

18-19: New Shutdown Hooks in Bullet List

The updated bullet list now correctly includes the new OnPreShutdown and OnPostShutdown hooks. Ensure that any references to the obsolete shutdown hook are fully removed from the documentation.


178-181: Address Multiple Consecutive Blank Lines

Static analysis flagged multiple consecutive blank lines (MD012) around this section (lines 179-180). Please remove the extra blank lines to comply with markdownlint guidelines. For example, consolidate the blank lines to a single one.

🧰 Tools
🪛 GitHub Check: markdownlint

[failure] 179-179: Multiple consecutive blank lines
docs/api/hooks.md:179 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md


[failure] 180-180: Multiple consecutive blank lines
docs/api/hooks.md:180 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 3] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md

🪛 GitHub Actions: markdownlint

[error] 179-179: MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3cb0c12 and 05e14f5.

📒 Files selected for processing (2)
  • docs/api/hooks.md (3 hunks)
  • listen_test.go (3 hunks)
🧰 Additional context used
🪛 GitHub Check: markdownlint
docs/api/hooks.md

[failure] 180-180: Multiple consecutive blank lines
docs/api/hooks.md:180 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 3] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md

🪛 GitHub Actions: markdownlint
docs/api/hooks.md

[error] 179-179: MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md

⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Compare
  • GitHub Check: repeated
🔇 Additional comments (5)
listen_test.go (2)

40-52: Well-structured test organization!

The test has been effectively split into three distinct scenarios with clear names and appropriate timeout values, improving readability and maintainability.


54-56: LGTM! Follows test helper best practices.

The helper function is properly declared with t.Helper() and has clear parameter names.

docs/api/hooks.md (3)

32-33: New Handler Type Declarations

The new type definitions for OnPreShutdownHandler and OnPostShutdownHandler are added and follow the expected signatures. This change aligns well with the split of the shutdown hook functionality.


180-185: Documentation for OnPreShutdown Hook

The OnPreShutdown section clearly describes the hook and provides an accurate function signature for registering pre-shutdown callbacks. This documentation meets the new design requirements and clarifies the hook’s intent.

🧰 Tools
🪛 GitHub Check: markdownlint

[failure] 180-180: Multiple consecutive blank lines
docs/api/hooks.md:180 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 3] https://github.com/DavidAnson/markdownlint/blob/v0.37.4/doc/md012.md


187-191: Documentation for OnPostShutdown Hook

The OnPostShutdown section is properly updated with a clear description and corresponding signature. This correctly informs users of its purpose in executing post-shutdown callbacks.

…ub.com:JIeJaitt/fiber into jiejaitt-feature/improve-shutdown-with-context
@JIeJaitt
Copy link
Contributor Author

@JIeJaitt last two doc changes and then i will merge

I have modified the document according to the discussion.

@ReneWerner87 ReneWerner87 merged commit 4b62d3d into gofiber:main Feb 22, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants