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

Fixes to the graceful shutdown example #2552

Merged
merged 4 commits into from
Jan 13, 2021
Merged

Fixes to the graceful shutdown example #2552

merged 4 commits into from
Jan 13, 2021

Conversation

jjba23
Copy link
Contributor

@jjba23 jjba23 commented Nov 11, 2020

…ce before the if statement on graceful shutdown

  • With pull requests:
    • Open your pull request against master
    • Your pull request should have no more than two commits, if not you should squash them.
    • It should pass all tests in the available continuous integration systems such as TravisCI.
    • You should add/modify tests to cover your proposed code changes.
    • If your pull request contains a new feature, please document it on the README.

@jjba23 jjba23 changed the title Change error comparison to use errors.Is() and add a line of whitespa… Fixes to the graceful shutdown example Nov 11, 2020
@jjba23
Copy link
Contributor Author

jjba23 commented Nov 11, 2020

As stated in the Shutdown function of http.Server:

// When Shutdown is called, Serve, ListenAndServe, and
// ListenAndServeTLS immediately return ErrServerClosed. Make sure the
// program doesn't exit and waits instead for Shutdown to return.

In the graceful shutdown example, the program would exit with log.Fatalf thus preventing the graceful shutdown.

@jjba23
Copy link
Contributor Author

jjba23 commented Jan 1, 2021

A project as Gin that is so loved, should really have some more active maintainers.. Can anyone give a comment on this?

@codecov
Copy link

codecov bot commented Jan 11, 2021

Codecov Report

Merging #2552 (b312eb2) into master (f4bc259) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2552   +/-   ##
=======================================
  Coverage   98.64%   98.64%           
=======================================
  Files          41       41           
  Lines        1989     1989           
=======================================
  Hits         1962     1962           
  Misses         15       15           
  Partials       12       12           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4bc259...b312eb2. Read the comment docs.

@hooman96
Copy link

gin needs additional wrapper for graceful shutdown! Currently, we only have Run() but there should be an additional function like RunGracefully() for such cases.

Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@thinkerou thinkerou added this to the 1.7 milestone Jan 13, 2021
@thinkerou thinkerou requested a review from appleboy January 13, 2021 01:05
@thinkerou thinkerou merged commit 46ddd42 into gin-gonic:master Jan 13, 2021
@tarupo
Copy link

tarupo commented Jan 26, 2021

@averageflow Sorry to border you, I just curious about why use errors.Is(...) but not using !errors.Is(...). Isn't it will ignore other errors? For example, I started 2 services on the same port(like 8080). And I will not get any errors message printed out, 2 services are still running but no errors are printed. It just looks like nothing going wrong. I think this is kind of weird.

	go func() {
		if err := srv.ListenAndServe(); err != nil && errors.Is(err, http.ErrServerClosed) {
			log.Printf("listen: %s\n", err)
		}
	}()

@jjba23
Copy link
Contributor Author

jjba23 commented Jan 26, 2021

@tarupo to be quite honest, I have altered the example to fit my needs and have a better solution. You also should not run things on the same port.

here are my utility functions:

const (
	GracefulShutdownRequestGraceSeconds = 10
)

// ListenAndServeGoRoutine will initiate the listen and serve of the wanted *http.Server and
// respond with a log when the server must be closed.
func ListenAndServeGoRoutine(srv *http.Server) {
	err := srv.ListenAndServe()
	if err != nil && errors.Is(err, http.ErrServerClosed) {
		log.Println("Gracefully closing server..")
	}
}

// TerminationSignalWatcher will wait for interrupt signal to gracefully shutdown
// the server with a timeout of x seconds.
func TerminationSignalWatcher(srv *http.Server) {
	// Make a channel to receive operating system signals.
	quit := make(chan os.Signal, 1)

	signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM, syscall.SIGABRT)
	<-quit
	log.Println("Shutting down server, because of received signal..")

	// The context is used to inform the server it has x seconds to finish
	// the request it is currently handling
	ctx, cancel := context.WithTimeout(
		context.Background(),
		GracefulShutdownRequestGraceSeconds*time.Second,
	)

	err := srv.Shutdown(ctx)
	if err != nil {
		log.Fatal("Server forced to shutdown:", err)
	}

	defer cancel()

	log.Println("Server exiting..")
}

and how you could call them in your main:

srv := &http.Server{
  ReadTimeout:  10 * time.Second,
  WriteTimeout: 30 * time.Second,
  IdleTimeout:  100 * time.Second,
  Addr:         ":7001",
  Handler:      router,
}

// Initialize the server in a goroutine so that
// it won't block the graceful shutdown handling below
go ListenAndServeGoRoutine(srv)

TerminationSignalWatcher(srv)

I feel like this is a better example of how you could get it to work, and can recommend it for everyone. I have this working wonderfully in production for several applications.

@jjba23
Copy link
Contributor Author

jjba23 commented Jan 26, 2021

@thinkerou maybe you can find some time to add my fonds to the example code and explanation.. i think it is a nicer and cleaner solution. But yes, Go and Gin are already providing you with the puzzle pieces, but i think it is nice for us to showcase it to newcomers

RK-GCP added a commit to RK-GCP/gin that referenced this pull request Mar 18, 2021
* Revert "Adding ppc64le architecture support on travis-ci (gin-gonic#2538)" (gin-gonic#2602)

* test: fixed the TestUnixSocket test on windows (gin-gonic#2595)

Co-authored-by: thinkerou <[email protected]>

* gin mode unknown: show available mode (gin-gonic#2567)

Co-authored-by: thinkerou <[email protected]>

* fix error gin support min Go version (gin-gonic#2584)

Co-authored-by: thinkerou <[email protected]>

* Fixes to the graceful shutdown example (gin-gonic#2552)

* Change error comparison to use errors.Is() and add a line of whitespace before the if statement on graceful shutdown

* Change from log.Fatalf to log.Printf to ensure the graceful shutdown actually works

Co-authored-by: J. J. Bigorra <[email protected]>
Co-authored-by: thinkerou <[email protected]>

* basic auth: fix timing oracle (gin-gonic#2609)

Co-authored-by: thinkerou <[email protected]>

* chore: Deleted spaces (gin-gonic#2622)

* Remove the tedious named return value (gin-gonic#2620)

Co-authored-by: thinkerou <[email protected]>

Co-authored-by: thinkerou <[email protected]>
Co-authored-by: Jeff <[email protected]>
Co-authored-by: Rubi <[email protected]>
Co-authored-by: Qt <[email protected]>
Co-authored-by: Josep Jesus Bigorra Algaba <[email protected]>
Co-authored-by: J. J. Bigorra <[email protected]>
Co-authored-by: Snawoot <[email protected]>
Co-authored-by: Alexander Melentyev <[email protected]>
Co-authored-by: Andy Pan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants