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

Feature: Implement Graceful shutdown #21

Merged
merged 34 commits into from
Oct 30, 2024
Merged

Conversation

werniq
Copy link
Contributor

@werniq werniq commented Oct 22, 2024

This PR introduces graceful shutdown functionality to the server. It ensures that the server properly handles shutdown signals (SIGINT, SIGTERM), stops accepting new requests, and allows ongoing requests to complete before shutting down. The update includes:

  • A new Shutdown method to gracefully stop the server with a timeout.
  • Signal handling in the Start and Run methods to listen for shutdown signals and trigger the graceful shutdown process.

This change enhances the server's stability by ensuring a smooth and controlled shutdown process.

select {
case sig := <-signalChan:
log.Printf("Received shutdown signal: %s, shutting down gracefully...", sig)
cancel()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see here, when we receive signal into signalChan, we call cancel func, which closes our context, and 'graceful-shutdowning' process begins.

go.mod Outdated
@@ -4,29 +4,31 @@ go 1.22.0

require (
github.com/Netflix/go-env v0.0.0-20220526054621-78278af1949d
github.com/cdipaolo/goml v0.0.0-20220715001353-00e0c845ae1c
github.com/corazawaf/libinjection-go v0.1.3
github.com/emirpasic/gods v1.18.1
github.com/goccy/go-yaml v1.11.3
Copy link
Contributor Author

@werniq werniq Oct 22, 2024

Choose a reason for hiding this comment

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

Had to run go mod tidy here, seems we have some unused imports left.

As an idea, we can add these to our CI/CD pipeline, and check if go mod tidy + git diff will return true, meaning that contributor should clean deps first.

@werniq
Copy link
Contributor Author

werniq commented Oct 22, 2024

@cebilon123 while repairing CI/CD I've noticed that some tests took really long. After some investigation, I've reached to rule package. I'm not sure, but I can't see any usage of it in the Waffle.

Is it needed? Or maybe it will be used later?

Taskfile.yaml Outdated
@@ -1,5 +1,8 @@
version: '3'

env:
IMAGE_NAME: '{{ .IMAGE_NAME | default "qniw984/waffle:1.0.0" }}'
Copy link
Owner

Choose a reason for hiding this comment

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

Why qniw984 tho? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to <YOUR_USERNAME>, so user can specify his/her username

}
}()

log.Println("Server started on :8080")
Copy link
Owner

Choose a reason for hiding this comment

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

I think it must be modified to the correct ports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like this:

image

Comment on lines 3 to 30
//import (
// "fmt"
// "golang.org/x/sys/windows"
// "os"
// "strings"
// "syscall"
//)
//
//func RunMeElevated() error {
// verb := "runas"
// exe, _ := os.Executable()
// cwd, _ := os.Getwd()
// args := strings.Join(os.Args[1:], " ")
//
// verbPtr, _ := syscall.UTF16PtrFromString(verb)
// exePtr, _ := syscall.UTF16PtrFromString(exe)
// cwdPtr, _ := syscall.UTF16PtrFromString(cwd)
// argPtr, _ := syscall.UTF16PtrFromString(args)
//
// var showCmd int32 = 1 //SW_NORMAL
//
// err := windows.ShellExecute(0, verbPtr, exePtr, argPtr, cwdPtr, showCmd)
// if err != nil {
// return fmt.Errorf("windows shell execute: %w", err)
// }
//
// return nil
//}
Copy link
Owner

Choose a reason for hiding this comment

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

You can remove the whole file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@werniq
Copy link
Contributor Author

werniq commented Oct 23, 2024

@thegodenage what about rule package? It is not used in project, but tests from this package are failing. Can I remove it too?

@werniq
Copy link
Contributor Author

werniq commented Oct 24, 2024

@thegodenage Finally, CI is blue 🙂

@werniq werniq requested a review from thegodenage October 24, 2024 10:07
@werniq
Copy link
Contributor Author

werniq commented Oct 24, 2024

@thegodenage where can we chat about helm? There are few things that I would like to know, before implementing it.

@thegodenage
Copy link
Owner

@werniq Hey sure, sorry for late response, have a lot of different stuff in the meantime regarding daytime job and apartment.

@thegodenage thegodenage merged commit 0df4ec6 into thegodenage:main Oct 30, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants