Handle the SIGINT and SIGTERM signals to support stopping a test cleanly

[hellais]

This fixes: #76

[bassosimone]

This is certainly a good start. I have a request to use some form of atomic semantic for the flag variable and I would also like to see a follow-up issue describing using a cancellable context.Context.

[bassosimone]

Please, use atomic to make sure we're accessing the variable with atomic semantics. I am thinking at something like atomic.AddInt64(&ctx.IsTerminated) to stop and atomic.LoadInt64(&ctx.IsTerminated) to fetch it.

Unfortunately there is no atomic boolean. If you wish to retain a boolan type, otherwise use a sync.Mutex but I have a mild preference for atomic semantics because it's more robust than explicitly using mutexes.

[bassosimone]

Please, document that the proper way to attain this objective would instead be to use a context.Context and cancel it when a signal is delivered, but this kind of refactoring is too complex for now. It would perhaps be top to have an issue that suggests to use a cancellable context rather than just adding a TODO comment to the code.

[hellais]

I have a request to use some form of atomic semantic for the flag variable and I would also like to see a follow-up issue describing using a cancellable context.Context.

I agree with you that using a context.Context object would probably be the optimal pattern, yet I think we would need to do quite some more refactoring in order to obtain that, so I suggest we defer that to future work (I took note of this inside of #45).

Regarding the atomic semantics for the flag, I am not sure I properly understand what race condition or inconsistency you are concerned about.
The IsTerminated bool variable is initialised once with the Context and is only every written once when the signal handler channel is triggered: https://github.com/ooni/probe-cli/pull/84/files#diff-be745cb99b81f008f99c3b674683ef40R29.

Throughout the rest of the code this variable is only ever read, so I am not really sure what we are gaining by using an atomic construct. For example if there is a "race" when the IsTerminated is written just in the moment that it's being read in https://github.com/ooni/probe-cli/pull/84/files#diff-be745cb99b81f008f99c3b674683ef40R49 or https://github.com/ooni/probe-cli/pull/84/files#diff-eacd810feae41332a4946c2af72b0b5aR110, that is not a problem. We will just do an extra loop and terminate cleanly on the next iteration.

Since the setup is that of a single writer, I don't see it possible for a race condition and I think using the atomic construct is unnecessary.

[hellais]

Following a private chat with @bassosimone I was enlightened by the magic that happens in the golang memory model, see: https://golang.org/ref/mem.

Some key takeways from that text include:

Worse, there is no guarantee that the write to done will ever be observed by main, since there are no synchronization events between the two threads. The loop in main is not guaranteed to finish.

and

The Go memory model specifies the conditions under which reads of a variable in one goroutine can be guaranteed to observe values produced by writes to the same variable in a different goroutine.

Moreover a minimal version of our code like this:

package main
import "time"
type Context struct {
        done bool
}
func main() {
	s := make(chan bool, 1)
	ctx := new(Context)
	go func() {
		<-s
		ctx.done = true
	}()
	go func() {
		time.Sleep(3 * time.Second)
		s <- true
	}()
	for true {
		if (ctx.done == true) {
			break
		}
		time.Sleep(1 * time.Second)
	}
}

results in a race being detected by the golang race detector.

[hellais]

I pushed changes that implement the recommended atomic patterns.

[bassosimone]

🐳