Review feedback

README.md

@@ -1,19 +1,27 @@
 # runsvinit [![Circle CI](https://circleci.com/gh/peterbourgon/runsvinit.svg?style=svg)](https://circleci.com/gh/peterbourgon/runsvinit)
 
-If you have a Docker container that's a collection of runit-supervised daemons,
+If you have a Docker container that's a collection of runit-supervised daemons, this process is suitable for use as the ENTRYPOINT.
-this process is suitable for use as the ENTRYPOINT.
 See [the example](https://github.com/peterbourgon/runsvinit/tree/master/example).
 
-**Why not just exec runsvdir?**
+**Why not use runit(8) directly?**
 
-`docker stop` issues SIGTERM (or, in a future version of Docker, perhaps another custom signal)
+[runit(8)](http://smarden.org/runit/runit.8.html) is designed to be used as process 1.
-but if runsvdir receives a signal,
+And, if you provide an `/etc/service/ctrlaltdel` script, it will be executed when runit receives the INT signal.
-it doesn't wait for its supervised processes to exit before returning.
+So, we could use that hook to gracefully terminate our services.
-If you don't care about graceful shutdown of your daemons, no problem, you don't need this tool.
+But Docker only sends TERM on `docker stop`.
 
-**Why not wrap runsvdir in a simple shell script?**
+Note that the container stop signal [will become configurable](https://github.com/docker/docker/pull/15307) in Docker 1.9.
+Once Docker 1.9 ships, this utility will be obsolete.
 
-This works great:
+**Why not just exec runsvdir(8) directly?**
+
+If [runsvdir(8)](http://smarden.org/runit/runsvdir.8.html) receives a signal, it doesn't wait for its supervised processes to exit before returning.
+
+**Why not wrap runsvdir(8) in a simple shell script?**
+
+Process 1 has the additional responsibility of [reaping orphaned child processes](https://blog.phusion.nl/2015/01/20/docker-and-the-pid-1-zombie-reaping-problem/).
+To the best of my knowledge, there is no sane way to do this with a shell script.
+Otherwise, indeed, this would work great:
 
 ```sh
 #!/bin/sh
@@ -30,14 +38,10 @@ trap "sv_stop; exit" SIGTERM
 wait
 ```
 
-...except it doesn't [reap orphaned child processes](https://blog.phusion.nl/2015/01/20/docker-and-the-pid-1-zombie-reaping-problem/)
-and is therefore unsuitable for being PID 1.
-
 **Why not use my_init from [phusion/baseimage-docker](https://github.com/phusion/baseimage-docker)?**
 
-That works great — if you're willing to add python3 to your Docker images :)
+my_init depends on Python 3, which might be a big dependency for such a small responsibility.
 
 **So this is just a stripped-down my_init in Go?**
 
 Basically, yes.
-

example/Makefile

@@ -2,9 +2,9 @@
 docker: runsvinit
 	docker build -t runsvinit-example .
 
-runsvinit: runsvinit-linux-amd64.tgz
+runsvinit: $GOPATH/bin/runsvinit
-	tar zxf $<
+	cp $< $@
 
-runsvinit-linux-amd64.tgz:
+$GOPATH/bin/runsvinit:
-	wget --quiet https://github.com/peterbourgon/runsvinit/releases/download/v2.0.0/runsvinit-linux-amd64.tgz
+	go get -u github.com/peterbourgon/runsvinit
 

main.go

@@ -8,6 +8,7 @@ import (
 	"os/signal"
 	"path/filepath"
 	"strings"
+	"sync"
 	"syscall"
 )
 
@@ -121,7 +122,7 @@ func shutdown(sv string, s signaler) {
 		return
 	}
 
-	var stopped []string
+	var wg sync.WaitGroup
 	for _, match := range matches {
 		fi, err := os.Stat(match)
 		if err != nil {
@@ -134,14 +135,19 @@ func shutdown(sv string, s signaler) {
 		}
 		service := filepath.Base(match)
 		stop := cmd(sv, "stop", service)
-		if err := stop.Run(); err != nil {
+		debugf("stopping %s...", service)
-			infof("%s: %v", strings.Join(stop.Args, " "), err)
+		wg.Add(1)
-			continue
+		go func() {
-		}
+			defer wg.Done()
-		stopped = append(stopped, service)
+			if err := stop.Run(); err != nil {
+				infof("%s: %v", strings.Join(stop.Args, " "), err)
+			} else {
+				debugf("stopped %s", service)
+			}
+		}()
 	}
+	wg.Wait()
 
-	debugf("stopped %d: %s", len(stopped), strings.Join(stopped, ", "))
 	debugf("stopping supervisor with signal %s...", sig)
 	if err := s.Signal(sig); err != nil {
 		info(err)

zombietest/Makefile

@@ -1,6 +1,6 @@
-GO?=go
+GO ?= go
-SUDO?=
+SUDO ?=
-RM?=--rm
+RMCONTAINER ?= --rm
 
 .PHONY: test clean
 
@@ -15,7 +15,7 @@ runsvinit: ../*.go
 	env GOOS=linux GOARCH=amd64 $(GO) build -o $@ github.com/peterbourgon/runsvinit
 
 zombie: .build.uptodate
-	$(SUDO) docker run $(RM) -v $(shell pwd):/mount zombietest-build cc -Wall -Werror -o /mount/zombie /zombie.c
+	$(SUDO) docker run $(RMCONTAINER) -v $(shell pwd):/mount zombietest-build cc -Wall -Werror -o /mount/zombie /zombie.c
 
 .build.uptodate: build/zombie.c build/Dockerfile
 	$(SUDO) docker build -t zombietest-build build/

zombietest/test.bash

@@ -11,14 +11,15 @@ function zombies() {
 }
 
 function stop_rm() {
-	docker stop $1
+	docker stop $1 >/dev/null
-	docker rm $1
+	#docker logs $1
+	docker rm $1 >/dev/null
 }
 
 SLEEP=1
 RC=0
 
-C=$(docker run -d zombietest /runsvinit -reap=false)
+C=$(docker run -d zombietest /runsvinit -reap=false -debug)
 sleep $SLEEP
 NOREAP=$(zombies)
 echo -n without reaping, we have $NOREAP zombies...
@@ -31,7 +32,7 @@ else
 fi
 stop_rm $C
 
-C=$(docker run -d zombietest /runsvinit)
+C=$(docker run -d zombietest /runsvinit -debug)
 sleep $SLEEP
 YESREAP=$(zombies)
 echo -n with reaping, we have $YESREAP zombies...