nsqd/nsqlookupd: exit when tcp server accept error

[andyxning]

This is a follow up PR for #1135 .

[ploxiln]

There's not much point in always returning the same error. Consider that this also happens for a normal exit (when it closes the Listener) and then because this returned an error, the wrapper func will call NSQD.Exit() again concurrently.

I suggest an NSQD "exiting" bit. In the wrapper (below in nsqd.go), when this function TCPServer finishes, if the exiting flag is already set, that's normal. If the exiting flag is not set, that is unexpected, and it should call NSQD.Exit().

[ploxiln]

... and if this wrapper function calls NSQD.Exit() it has to launch a separate goroutine for it, because it will deadlock with the waitGroup waiting for this wrapper function to finish.

[andyxning]

Good catch. Will investigate

[andyxning]

@ploxiln PTAL.

[ploxiln]

You could just return fatalErr in all cases, it will be nil if never set.

But actually, I suggest reverting this function to not return an error at all.

[ploxiln]

well, thinking about it, it might still be a good idea to return fatalErr, for nsqlookupd to do the ungraceful abort of os.Exit(1)

[ploxiln]

I suggest checking the exiting flag here, and if it is not already set, then go Exit().

The flag variable could be an atomic int, to avoid the need to take a lock.

[ploxiln]

(this suggestion would require Exit() to set the flag as the first thing it does)

[andyxning]

I suggest we keep the check logic be within the Exit function. With this we should not take care about the internal exit status flag check before Exit is called in other places. And we should keep Exit function to be called only once. Without lock protection, even with atomic values, the set and check logic exists race condition.

With setting and checking separated with aotmic values, race condition may happens. For example, a normal exit and accept error exit happens simultaneously, before the normal exit call set the Exit flag, accept error exit checks it and then call Exit again. Although nsqd will exit correctly but Exit is called more than once.

With lock like this. Exit function logic except closing listeners will only be called once.

[andyxning]

BTW, lock consumption should be ok and will not invoke any new performance problems, because all this happens under exit phase.

[ploxiln]

The reason I suggested the exiting flag early on is because deciding to call Exit here could be as simple as:

--- a/nsqd/nsqd.go
+++ b/nsqd/nsqd.go
@@ -71,6 +71,7 @@ type NSQD struct {
 
        notifyChan           chan interface{}
        optsNotificationChan chan struct{}
+       exitFlag             int32
        exitChan             chan int
        waitGroup            util.WaitGroupWrapper
 
@@ -264,6 +265,10 @@ func (n *NSQD) Main() {
        tcpServer := &tcpServer{ctx: ctx}
        n.waitGroup.Wrap(func() {
                protocol.TCPServer(n.tcpListener, tcpServer, n.logf)
+               if atomic.LoadInt32(&t.exitFlag) == 0 {
+                       // abnormal listen loop exit
+                       go n.Exit()
+               }
        })
        httpServer := newHTTPServer(ctx, false, n.getOpts().TLSRequired == TLSRequired)
        n.waitGroup.Wrap(func() {
@@ -423,6 +428,10 @@ func (n *NSQD) PersistMetadata() error {
 }
 
 func (n *NSQD) Exit() {
+       if !atomic.CompareAndSwapInt32(&t.exitFlag, 0, 1) {
+               return
+       }
+
        if n.tcpListener != nil {
                n.tcpListener.Close()
        }

(but there still is the issue of exiting with an error code)

[mreiferson]

Agreed that the atomic would be slightly cleaner, but I prefer directing flow through the existing machinery that's setup to manage the lifecycle of the service to ensure that, whatever svc.Run is doing, it gets done (rather than introducing a new exceptional path).

[ploxiln]

yeah ... calling NSQD.Exit() cleans up but does not actually exit ... go-svc needs to be involved anyway

[ploxiln]

one more cheesy idea to avoid needing to deal with go-svc:

--- a/nsqd/nsqd.go
+++ b/nsqd/nsqd.go
@@ -264,6 +264,12 @@ func (n *NSQD) Main() {
        tcpServer := &tcpServer{ctx: ctx}
        n.waitGroup.Wrap(func() {
                protocol.TCPServer(n.tcpListener, tcpServer, n.logf)
+               if atomic.LoadInt32(&t.exitFlag) == 0 {
+                       go func() {
+                               n.Exit()
+                               os.Exit(1)
+                       }()
+               }
        })

[mreiferson]

Meh, let's just do it the "right" way.

[mreiferson]

An alternative way to approach this, that would avoid having to protect Exit() from multiple calls would be to create a fatalErrCh channel, that gets passed to svc.Run. We could close() it here.

[mreiferson]

If we take the approach I proposed, this log line should communicate that the error is FATAL.

[andyxning]

Done.

[ploxiln]

should unlock if returning here

[andyxning]

Good catch.

[mreiferson]

Let's finish this up in #1140, thanks @andyxning