nsqd/nsqlookupd: properly handle fatal accept errors

[mdh67899]

#1138

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]

Thanks for taking a pass at this, I think we can make this a slightly smaller set of changes and still achieve the desired result.

[mdh67899]

/ping @ploxiln @mreiferson. PTAL. :)

[ploxiln]

it looks to me like, after all this, we ended up with something equivalent to #1138 (comment) (check if already doing exit, call NSQD.Exit(), call os.Exit(1))

[mreiferson]

Indeed, thank you for pointing that out. I still feel like the application (not nsqd "library") should have the responsibility of handling "exiting" and that the library is just missing a way to signal that.

I'm going to try to modify this PR on my own to see if I can get it to a place where I'm reasonably happy with it and then we can discuss.

[ploxiln]

I do have one more idea: send SIGINT to getpid(). Not sure if it works on windows though.

[mreiferson]

pushed up my changes, if this seems reasonable I'll apply the same approach to nsqlookupd

[mreiferson]

(it does not yet even compile, mostly due to logging related bullshit)

took over PR

[ploxiln]

NSQD.New() returning error makes good sense.

NSQD.Main() becoming synchronous ... is going to change things for tests

[mreiferson]

NSQD.Main() becoming synchronous ... is going to change things for tests

Yea, for sure, I'll clean all that up. I think the semantics of it being synchronous make more sense to me. I've come around to the idea that libraries shouldn't try to own concurrency in this way, and we've made this mistake in a few places (mostly thinking go-nsq). Also, we've never promised an API guarantee for nsqd as a library, so we don't need to consider backwards compat.

[ploxiln]

I do like the "return err instead of just os.Exit(1)" pattern
(as you say, so the caller can handle appropriately)

[andyxning]

I do have one more idea: send SIGINT to getpid(). Not sure if it works on windows though.

This is what i think, also. This way we do not need to any api.

[ploxiln]

This way looks OK to me.

[mreiferson]

Deeper down the rabbit hole...

Commit d6b87a5 improves how LogLevel is parsed (moves responsibility to application side flag parsing, and uses strict types on the nsqd side Options struct). This required changes to go-options.

Commit 76f3702 should start to fix tests, which required moving some initializing to New(), which I'm interested in feedback on.

[mreiferson]

Finally have a green test run, PTAL and I'll clean up these commits.

[ploxiln]

$ ./build/nsqadmin --verbose --nsqd-http-address=http://127.0.0.1:4151
panic: interface conversion: *app.StringArray is not flag.Getter: missing method Get

goroutine 1 [running]:
github.com/nsqio/nsq/vendor/github.com/mreiferson/go-options.Resolve(0x13a69a0, 0xc0000cc580, 0xc00002e360, 0x0)
	/Users/pierce/go/src/github.com/nsqio/nsq/vendor/github.com/mreiferson/go-options/options.go:86 +0xa3e
main.(*program).Start(0xc00000c440, 0x14be420, 0x17e0bf0)
	/Users/pierce/go/src/github.com/nsqio/nsq/apps/nsqadmin/main.go:105 +0x38a
github.com/nsqio/nsq/vendor/github.com/judwhite/go-svc/svc.Run(0x14c0e40, 0xc00000c440, 0xc00000c460, 0x2, 0x2, 0xc000066f20, 0x0)
	/Users/pierce/go/src/github.com/nsqio/nsq/vendor/github.com/judwhite/go-svc/svc/svc_other.go:20 +0x74
main.main()
	/Users/pierce/go/src/github.com/nsqio/nsq/apps/nsqadmin/main.go:72 +0xbf

[mreiferson]

Sonofa! Thanks, will fix.

[mreiferson]

OK, cleaned up the commits 🙏

[ploxiln]

As far as I can tell, this is pretty good. It's not easy to trigger the failure paths ... this is as far as I got with very low ulimit and parallel http requests:

$ ./build/nsqd --tcp-address 0.0.0.0:530 --log-level=debug
[nsqd] 2019/03/02 11:27:03.212215 INFO: nsqd v1.1.1-alpha (built w/go1.11.5)
[nsqd] 2019/03/02 11:27:03.212320 INFO: ID: 136
[nsqd] 2019/03/02 11:27:03.212645 INFO: NSQ: persisting topic/channel metadata to nsqd.dat
[nsqd] 2019/03/02 11:27:03.214231 INFO: TCP: listening on [::]:530
[nsqd] 2019/03/02 11:27:03.214297 INFO: HTTP: listening on [::]:4151
[nsqd] 2019/03/02 11:27:21.991592 WARNING: http: Accept error: accept tcp [::]:4151: accept: too many open files; retrying in 5ms
[nsqd] 2019/03/02 11:27:29.912527 INFO: 200 GET /stats ([::1]:56231) 214.582µs
[nsqd] 2019/03/02 11:27:41.040258 WARNING: http: Accept error: accept tcp [::]:4151: accept: too many open files; retrying in 5ms
[nsqd] 2019/03/02 11:27:45.535751 WARNING: http: Accept error: accept tcp [::]:4151: accept: too many open files; retrying in 10ms
[nsqd] 2019/03/02 11:27:45.814051 WARNING: http: Accept error: accept tcp [::]:4151: accept: too many open files; retrying in 20ms
[nsqd] 2019/03/02 11:27:46.626209 WARNING: http: Accept error: accept tcp [::]:4151: accept: too many open files; retrying in 40ms
[nsqd] 2019/03/02 11:27:52.394460 INFO: 200 GET /stats ([::1]:56230) 204.084µs

[ploxiln]

other than that 👍

[judwhite]

@mreiferson It doesn't hurt anything to have this, but the package does this bit for you now. I generally consider it impolite to os.Chdir but in this case I thought it was warranted. https://github.com/judwhite/go-svc/blame/d83e900e4a688aad49f41263bebf8793f0bf6add/svc/svc_windows.go#L59-L66