e.StartServer() vs. e.Close() etc.

[sebogh]

Issue Description

If e.StartServer() was used to start a custom http server, e.Close() and e.Shutdown() will not work as expected.

The problem seems to be that e.Close() and e.Shutdown() friends act on e.Server which is not updated by e.StartServer(). This was raised back in #1266 and is somewhat related to #1793.

Checklist

• [ x] Dependencies installed
• [ x] No typos
• [ x] Searched existing issues and docs

Expected behaviour

e.Close() and e.Shutdown() should tear down servers started via e.StartServer().

Actual behaviour

e.Close() and e.Shutdown() tear down the default server.

Steps to reproduce

Working code to debug

func TestEchoShutdown2(t *testing.T) {

	e := New()
	e.HideBanner = true
	e.HidePort = true

	go func() {
		s := &http.Server{Addr: ":8080"}
		e.StartServer(s)
	}()

	// give the server some time to come up
	time.Sleep(200 * time.Millisecond)

	// query the server expecting a 404 but no error
	client := &http.Client{}
	_, err := client.Get("http://localhost:8080")
	assert.NoError(t, err)

	// close the server
	e.Close()

	// query again expect result connection refused error
	_, err = client.Get("http://localhost:8080")
	assert.Error(t, err)
}

execute with:

go test -run TestEchoShutdown2 

Version/commit

6f9b71c

[sebogh]

Wouldn't be easier to simply set e.Server and get rid of e.StartServer()?

[aldas]

There are other problems. 1 server can actually serve multiple listeners etc. echo instance contains 2 servers. So if you call close/shutdown and TLS fails to stop then regular server methods are not called.

as close and shutdown are just wrappers I think all server/listener instances should be removed from echo instance and current methods e.Start e.StartTLS e.StartAutoTLS e.StartServer e.StartH2CServer should be replaced with functions returning prepared http.Server (not serving yet) with echo set as handler and logger set as logger.

I would say their value is making developer life simpler for simple use cases but for more complex having server/listener embedded in echo instance it makes life harder. As soon you want to use your own server and/or listener(s) current Echo server related API methods are probably not for you - these were made for simpler cases long time ago and have evolved by adhoc patches over time.

but these are not backwards compatible changes and are not going to happen soon

You can do everything with just server instance. no need to call echo server related methods

e := New()

e.GET("/", func(c Context) error {
	return c.String(http.StatusOK, "OK")
})

// From here on work only with http.Server instance

server := http.Server{
	Addr:    ":8080",
	Handler: e,
}
go func() {
	if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
		e.Logger.Fatal("shutting down the server")
	}
}()

quit := make(chan os.Signal, 1)
signal.Notify(quit, os.Interrupt)
<-quit
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
if err := server.Shutdown(ctx); err != nil {
	e.Logger.Fatal(err)
}

[sebogh]

as close and shutdown are just wrappers I think all server/listener instances should be removed from echo instance and current methods e.Start e.StartTLS e.StartAutoTLS e.StartServer e.StartH2CServer should be replaced with functions returning prepared http.Server (not serving yet) with echo set as handler and logger set as logger.

I agree.

I would say their value is making developer life simpler for simple use cases but for more complex having server/listener embedded in echo instance it makes life harder. As soon you want to use your own server and/or listener(s) current Echo server related API methods are probably not for you - these were made for simpler cases long time ago and have evolved by adhoc patches over time.

Again, I agree and see the value for a painless start.

[lammel]

I think we can close this issue.
There will be some changes to handling of startup in v5 using a dedicated StartupConfig.
See also #2000.