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

net/http: something about semantics of Server.Close #17880

Closed
peter-mogensen opened this issue Nov 10, 2016 · 4 comments
Closed

net/http: something about semantics of Server.Close #17880

peter-mogensen opened this issue Nov 10, 2016 · 4 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@peter-mogensen
Copy link

peter-mogensen commented Nov 10, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

Go master HEAD 8d0c105 + fix for #17878

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH=""
GORACE=""
GOROOT="/usr/lib/go"
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GO15VENDOREXPERIMENT="1"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0"
CXX="g++"
CGO_ENABLED="1"

What did you do?

package main

import (
	"log"
	"fmt"
	"time"
	"net"
	"net/http"
)

type server3 struct {
	http.Server
}

func (s *server3) Serve(l net.Listener) error {
	fmt.Println("Serving")
	// Remove sleep to not hit deadlock
	time.Sleep(time.Second)
	return s.Server.Serve(l)
}

func (s *server3) Shutdown() {
	fmt.Println("Closing")
	s.Close()
}

func main() {

	srv := &server3{}
	srvEnd :=  make(chan error)


	for {
		l, err := net.Listen("tcp","")
		if err != nil {
			log.Fatal(err)
		}
		
		go func() {
			err := srv.Serve(l)
			srvEnd <- err 
		}()

		time.Sleep(time.Second)
		
		srv.Shutdown()

		fmt.Println("Close done")
		
		err = <- srvEnd
		
		fmt.Println(err)
	}
}

What did you expect to see?

Serving
Closing
Close done
http: Server closed
Serving
Closing
Close done
http: Server closed
Serving
Closing
Close done
http: Server closed
Serving
Closing
Close done
http: Server closed
....

What did you see instead?

Serving
Closing
Close done
^C

Discussion

It seems that there's really no way to have guarantee whether a given Shutdown() will actually result in no running server.
The issue was discussed in this thread:
https://groups.google.com/forum/#!topic/golang-nuts/hACrtl4JoUY

@bradfitz
Copy link
Contributor

Please write a self-contained bug report that isn't dependent on reading a whole thread elsewhere.

What is the problem?

@bradfitz bradfitz added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Nov 10, 2016
@bradfitz bradfitz changed the title Racy semantics of http.Server.Close() ? net/http: something about semantics of Server.Close Nov 10, 2016
@peter-mogensen
Copy link
Author

The problem is that if you have a Server object which can be started and stopped and is restartable (which appears to be how http.Server{} is meant), then an async shutdown signal like Close() which closes anything the server does at the moment it takes the lock can't really be sure which Serve() invocation(s) it actually closed since their might be Serve() invocations called by not yet having effect.

It is of course a matter of subjective intention how it's meant to work and could be documented, but it should probably be a conscious decision.

If http.Serve was not restartable (or only restartable after at Reset() method had been called) then you would know in the above example that waiting would not deadlock.
But since it's restartable any Close() before Serve() takes the mutex lock is effectively a dropped message - has no effect. So whether or not you end up stopping a Serve() depends on timing. (as illustrated by adding a Sleep() to the Serve() call.

So (in effect) - if you hook http.Server.Close() up on an OS signal like SIGTERM then those signals would some times be lost due to them arriving at a point in a restart loop where there just was another Close() having closed the server and the restart already in progress but no listeners having been tracked yet.
Say you have SIGHUP triggering a Close() and a new Serve(), but have SIGINT triggering a Close() and and exit. Then a SIGINT arriving directly after at SIGHUP would exit while a server was still running since it's Close() invocation would not do anything.

One proposed alternative semantic would be to be able to associated a Serve() invocation with a context and having Close() cancel that context. Then you would always know exactly which Serve() invocations you closed ... of course that could be a complex API solution given the API promise.

This might be deliberately intended behavior ...

PS: Thanks to Nick Patavalis for bringing up the question on the list and figuring out most of the implications.

@bradfitz
Copy link
Contributor

Close is a really strong hammer. It interrupts requests still being processed. You can wire it up how you want to wire it up. Yes, it'll probably involve some coordination and synchronization on your side.

Shutdown is a much softer tool. But it also requires some coordination, depending on what you want to do.

If you want to close a specific Server.Serve, you already can: retain the net.Listener you pass to it, and call Close on it yourself.

There won't be any one magical answer for everybody that doesn't involve some code.

I'm going to close this because I'm not sure what there is to do here. I'm probably not understanding something still.

@peter-mogensen
Copy link
Author

Keeping the listener and calling Close() on it to cancel a specific Serve() invocation would solve part of the problem, yes ... however the result from the caller of Serve() viewpoint would be different in the ErrServerClosed would not be returned. .. which ties into #4373

@golang golang locked and limited conversation to collaborators Nov 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

3 participants