Fix hang for --discover-log-location flag

[msakrejda]

This was missed in #384.

[msakrejda]

I don't know if this is a good fix (this isn't really a "testRun"), but it works, and I don't have any better ideas. Thoughts?

[seanlinsley]

It looks like this is happening because the main function selects from this channel, and selecting from a nil channel will block forever. It seems Go's recommended solution is to add a timeout to that select, but we wouldn't want to confuse users with a timeout error when the function is returning normally.

So since select statements apparently can't check if the channel is nil first, this is the only way for the main function to handle it:

	DoneOrSignal:
		for {
			if testRunSuccess != nil {
				select {
				case success := <-testRunSuccess:
					if reloadRun {
						if success {
							Reload(logger)
						} else {
							logger.PrintError("Error: Reload requested, but ignoring since configuration errors are present")
							exitCode = 1
						}
					} else if !success {
						exitCode = 1
					}
					break DoneOrSignal
				case s := <-sigs:
					if s == syscall.SIGINT || s == syscall.SIGTERM {
						logger.PrintError("Interrupt")
						break DoneOrSignal
					}
				}
			} else {
				select {
				case s := <-sigs:
					if s == syscall.SIGINT || s == syscall.SIGTERM {
						logger.PrintError("Interrupt")
						break DoneOrSignal
					}
				}
			}
		}

I don't think either option is easy to understand, so your approach is probably better since it results in fewer lines of code.

[msakrejda]

Right, I think the underlying bug here is that we expect anything that exits quickly, instead of continuing to run, to be a test run and depend on reading something from the testRun channel (or a signal). I think a proper fix involves rethinking that, but the run method is kind of daunting, and I'd rather fix this first and refactor that some other time.

[seanlinsley]

It looks like this is happening because the main function selects from this channel, and selecting from a nil channel will block forever. It seems Go's recommended solution is to add a timeout to that select, but we wouldn't want to confuse users with a timeout error when the function is returning normally.

So since select statements apparently can't check if the channel is nil first, this is the only way for the main function to handle it:

	DoneOrSignal:
		for {
			if testRunSuccess != nil {
				select {
				case success := <-testRunSuccess:
					if reloadRun {
						if success {
							Reload(logger)
						} else {
							logger.PrintError("Error: Reload requested, but ignoring since configuration errors are present")
							exitCode = 1
						}
					} else if !success {
						exitCode = 1
					}
					break DoneOrSignal
				case s := <-sigs:
					if s == syscall.SIGINT || s == syscall.SIGTERM {
						logger.PrintError("Interrupt")
						break DoneOrSignal
					}
				}
			} else {
				select {
				case s := <-sigs:
					if s == syscall.SIGINT || s == syscall.SIGTERM {
						logger.PrintError("Interrupt")
						break DoneOrSignal
					}
				}
			}
		}

I don't think either option is easy to understand, so your approach is probably better since it results in fewer lines of code.