Skip to content

Conversation

@amenzhinsky
Copy link

No description provided.

@amenzhinsky amenzhinsky changed the title Add graceful stop Add graceful stop (go 1.8) Feb 17, 2017
@coveralls
Copy link

coveralls commented Feb 17, 2017

Coverage Status

Coverage increased (+0.1%) to 85.371% when pulling 668d551 on amenzhinsky:graceful-stop into 394f533 on labstack:master.

Copy link
Member

@vishr vishr left a comment

Choose a reason for hiding this comment

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

Rather than removing TLS*, you should check for e.Listener and e.TLSListener for nil and shutdown.

@amenzhinsky
Copy link
Author

@vishr I thought of that, but didn't figure out why they're needed, I mean TLS* fields. At the same time this is a code simplification, we can check when TLS is enabled via e.Server.TLSConfig != nil

@vishr
Copy link
Member

vishr commented Feb 17, 2017

@amenzhinsky They are required for the same reason as Server. Imagine when you are running both http and https and you need to have access to a Server object.

@amenzhinsky
Copy link
Author

@vishr so we need to stop them both, is that correct?

It'll look like:

func (e *Echo) Close() (err error) {
	if e.TLSListener != nil {
		if err = e.TLSServer.Close(); err != nil {
			return
		}
	}

	if e.Listener != nil {
		if err = e.Server.Close(); err != nil {
			return
		}
	}

	return 
}

@vishr
Copy link
Member

vishr commented Feb 18, 2017

Looks good to me. Also, you can just return e.TLSServer.Close().

@coveralls
Copy link

coveralls commented Feb 19, 2017

Coverage Status

Coverage increased (+0.03%) to 84.998% when pulling 21fb717 on amenzhinsky:graceful-stop into 0b53f39 on labstack:master.

@amenzhinsky
Copy link
Author

@vishr I've just updated this PR, please have a look.

I still think that having multiple server instances is not such a good idea because we use only (*http.Server) Serve(net.Listener) function that completely ignores TLSConfig since we provide it with a listener.

In this case we can listen to multiple ports TLS or plain, having only one server instance.
For compatibility issues we can use the same pointer for both Server and TLSServer

What's your take on that?

@vishr
Copy link
Member

vishr commented Feb 19, 2017

I think Server and TLSServer should be isolated, consider a case when you modify s.Addr in the code.

How do you use (*http.Server) Serve(net.Listener)? Can you do it by setting e.Listener or e.TLSListener?

@coveralls
Copy link

coveralls commented Feb 19, 2017

Coverage Status

Coverage decreased (-0.2%) to 84.798% when pulling b072207 on amenzhinsky:graceful-stop into a098bcd on labstack:master.

@amenzhinsky
Copy link
Author

Okay then, I made it simple for now.
I'll think of that a bit later.

@vishr
Copy link
Member

vishr commented Feb 20, 2017

@amenzhinsky This is merged with e7c89c4, somehow I lost your commit but the code is in. Thanks for your contribution.

@vishr vishr closed this Feb 20, 2017
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.

3 participants