From 748374eb07c2d06d65b5cc6be4986f7333781a53 Mon Sep 17 00:00:00 2001 From: Tom Wiesing Date: Mon, 7 Jun 2021 12:53:00 -0400 Subject: [PATCH 1/9] Protect env against concurrent access When multiple processes were immediatly crashing on startup, the environment map was written concurrently in multiple goroutines. This caused forego to crash in certain situations. To protect against this behavior, this commit protects multiple concurrent accesses by using a sync.Map instead. --- env.go | 56 ++++++++++++++++++++++++++++++++++++++++----------- env_test.go | 4 ++-- process.go | 4 ++-- start.go | 12 ++++++----- start_test.go | 6 +++--- 5 files changed, 58 insertions(+), 24 deletions(-) diff --git a/env.go b/env.go index 61ec535..6d4d0b8 100644 --- a/env.go +++ b/env.go @@ -4,13 +4,37 @@ import ( "fmt" "os" "regexp" + "sync" "github.com/subosito/gotenv" ) var envEntryRegexp = regexp.MustCompile("^([A-Za-z_0-9]+)=(.*)$") -type Env map[string]string +type Env struct { + m sync.Map +} + +func (e *Env) Get(key string) string { + value, ok := e.m.Load(key) + if !ok { + return "" + } + return value.(string) +} + +func (e *Env) Set(key, value string) { + e.m.Store(key, value) +} + +func (e *Env) Delete(key string) { + e.m.Delete(key) +} + +// NewEnv makes a new environment +func NewEnv() *Env { + return &Env{sync.Map{}} +} type envFiles []string @@ -23,13 +47,14 @@ func (e *envFiles) Set(value string) error { return nil } -func loadEnvs(files []string) (Env, error) { +func loadEnvs(files []string) (*Env, error) { if len(files) == 0 { files = []string{".env"} } - env := make(Env) + env := NewEnv() + // don't need to lock either environment for _, file := range files { tmpEnv, err := ReadEnv(file) @@ -38,25 +63,29 @@ func loadEnvs(files []string) (Env, error) { } // Merge the file I just read into the env. - for k, v := range tmpEnv { - env[k] = v - } + tmpEnv.m.Range(func(key, value interface{}) bool { + env.m.Store(key, value) + return true + }) } return env, nil } -func ReadEnv(filename string) (Env, error) { +func ReadEnv(filename string) (*Env, error) { + env := NewEnv() + if _, err := os.Stat(filename); os.IsNotExist(err) { - return make(Env), nil + return env, nil } + fd, err := os.Open(filename) if err != nil { return nil, err } defer fd.Close() - env := make(Env) + for key, val := range gotenv.Parse(fd) { - env[key] = val + env.Set(key, val) } return env, nil } @@ -65,8 +94,11 @@ func (e *Env) asArray() (env []string) { for _, pair := range os.Environ() { env = append(env, pair) } - for name, val := range *e { + + e.m.Range(func(name, val interface{}) bool { env = append(env, fmt.Sprintf("%s=%s", name, val)) - } + return true + }) + return } diff --git a/env_test.go b/env_test.go index 4eac3ae..7db1012 100644 --- a/env_test.go +++ b/env_test.go @@ -10,11 +10,11 @@ func TestMultipleEnvironmentFiles(t *testing.T) { t.Fatalf("Could not read environments: %s", err) } - if env["env1"] == "" { + if env.Get("env1") == "" { t.Fatal("$env1 should be present and is not") } - if env["env2"] == "" { + if env.Get("env2") == "" { t.Fatal("$env2 should be present and is not") } } diff --git a/process.go b/process.go index ae1bcdd..9725caf 100644 --- a/process.go +++ b/process.go @@ -8,13 +8,13 @@ import ( type Process struct { Command string - Env Env + Env *Env Interactive bool *exec.Cmd } -func NewProcess(workdir, command string, env Env, interactive bool) (p *Process) { +func NewProcess(workdir, command string, env *Env, interactive bool) (p *Process) { argv := ShellInvocationCommand(interactive, workdir, command) return &Process{ command, env, interactive, exec.Command(argv[0], argv[1:]...), diff --git a/start.go b/start.go index 59f5e47..855dfec 100644 --- a/start.go +++ b/start.go @@ -169,18 +169,19 @@ func (f *Forego) monitorInterrupt() { } } -func basePort(env Env) (int, error) { +func basePort(env *Env) (int, error) { + if flagPort != defaultPort { return flagPort, nil - } else if env["PORT"] != "" { - return strconv.Atoi(env["PORT"]) + } else if port := env.Get("PORT"); port != "" { + return strconv.Atoi(port) } else if os.Getenv("PORT") != "" { return strconv.Atoi(os.Getenv("PORT")) } return defaultPort, nil } -func (f *Forego) startProcess(idx, procNum int, proc ProcfileEntry, env Env, of *OutletFactory) { +func (f *Forego) startProcess(idx, procNum int, proc ProcfileEntry, env *Env, of *OutletFactory) { port, err := basePort(env) if err != nil { panic(err) @@ -192,7 +193,8 @@ func (f *Forego) startProcess(idx, procNum int, proc ProcfileEntry, env Env, of workDir := filepath.Dir(flagProcfile) ps := NewProcess(workDir, proc.Command, env, interactive) procName := fmt.Sprint(proc.Name, ".", procNum+1) - ps.Env["PORT"] = strconv.Itoa(port) + + ps.Env.Set("PORT", strconv.Itoa(port)) ps.Stdin = nil diff --git a/start_test.go b/start_test.go index d196f2c..1772052 100644 --- a/start_test.go +++ b/start_test.go @@ -111,7 +111,7 @@ func TestParseConcurrencyFlagNoValue(t *testing.T) { } func TestPortFromEnv(t *testing.T) { - env := make(Env) + env := NewEnv() port, err := basePort(env) if err != nil { t.Fatalf("Can not get base port: %s", err) @@ -129,7 +129,7 @@ func TestPortFromEnv(t *testing.T) { t.Fatal("Base port should be 4000") } - env["PORT"] = "6000" + env.Set("PORT", "6000") port, err = basePort(env) if err != nil { t.Fatalf("Can not get base port: %s", err) @@ -138,7 +138,7 @@ func TestPortFromEnv(t *testing.T) { t.Fatal("Base port should be 6000") } - env["PORT"] = "forego" + env.Set("PORT", "forego") port, err = basePort(env) if err == nil { t.Fatalf("Port 'forego' should fail: %s", err) From a9b926b018602d84f640a19052f4d89f154ba24b Mon Sep 17 00:00:00 2001 From: Tom Wiesing Date: Mon, 7 Jun 2021 12:57:19 -0400 Subject: [PATCH 2/9] Remove unuused variables This commit removes variables that are not used. --- command.go | 1 - env.go | 3 --- start_test.go | 2 +- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/command.go b/command.go index 2a275d7..4020e46 100644 --- a/command.go +++ b/command.go @@ -6,7 +6,6 @@ import ( "strings" ) -var flagEnv string var flagProcfile string type Command struct { diff --git a/env.go b/env.go index 6d4d0b8..81c6498 100644 --- a/env.go +++ b/env.go @@ -3,14 +3,11 @@ package main import ( "fmt" "os" - "regexp" "sync" "github.com/subosito/gotenv" ) -var envEntryRegexp = regexp.MustCompile("^([A-Za-z_0-9]+)=(.*)$") - type Env struct { m sync.Map } diff --git a/start_test.go b/start_test.go index 1772052..1b4cdd5 100644 --- a/start_test.go +++ b/start_test.go @@ -139,7 +139,7 @@ func TestPortFromEnv(t *testing.T) { } env.Set("PORT", "forego") - port, err = basePort(env) + _, err = basePort(env) if err == nil { t.Fatalf("Port 'forego' should fail: %s", err) } From c5c51a56a3050625c2f3a5061cb6a5a9bbb54623 Mon Sep 17 00:00:00 2001 From: Tom Wiesing Date: Mon, 7 Jun 2021 12:59:33 -0400 Subject: [PATCH 3/9] Replace c.Disabled != true with !c.Disabled --- command.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command.go b/command.go index 4020e46..74c13f4 100644 --- a/command.go +++ b/command.go @@ -36,7 +36,7 @@ func (c *Command) Name() string { } func (c *Command) Runnable() bool { - return c.Run != nil && c.Disabled != true + return c.Run != nil && !c.Disabled } func (c *Command) List() bool { From 6e45f4b148640fe44a2e56b140a2ee8decd1b86f Mon Sep 17 00:00:00 2001 From: Tom Wiesing Date: Mon, 7 Jun 2021 13:00:06 -0400 Subject: [PATCH 4/9] Replace loop with a single call to append() --- env.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/env.go b/env.go index 81c6498..396cf3b 100644 --- a/env.go +++ b/env.go @@ -88,9 +88,7 @@ func ReadEnv(filename string) (*Env, error) { } func (e *Env) asArray() (env []string) { - for _, pair := range os.Environ() { - env = append(env, pair) - } + env = append(env, os.Environ()...) e.m.Range(func(name, val interface{}) bool { env = append(env, fmt.Sprintf("%s=%s", name, val)) From 6fee385c828125e88bdd4c87a0eaaa273e4e4a12 Mon Sep 17 00:00:00 2001 From: Tom Wiesing Date: Mon, 7 Jun 2021 13:01:03 -0400 Subject: [PATCH 5/9] Remove redundant "return" statement --- unix.go | 1 - 1 file changed, 1 deletion(-) diff --git a/unix.go b/unix.go index 84f158c..c898b7e 100644 --- a/unix.go +++ b/unix.go @@ -23,7 +23,6 @@ func (p *Process) PlatformSpecificInit() { p.SysProcAttr = &syscall.SysProcAttr{} p.SysProcAttr.Setsid = true } - return } func (p *Process) SendSigTerm() { From 9d9373cb50e99b0be79dc9ba5ced45631fafeda1 Mon Sep 17 00:00:00 2001 From: Tom Wiesing Date: Mon, 7 Jun 2021 13:01:32 -0400 Subject: [PATCH 6/9] Don't capitalize error messages --- procfile.go | 2 +- start.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/procfile.go b/procfile.go index 07b9837..42561c8 100644 --- a/procfile.go +++ b/procfile.go @@ -68,7 +68,7 @@ func parseProcfile(r io.Reader) (*Procfile, error) { } } if err := scanner.Err(); err != nil { - return nil, fmt.Errorf("Reading Procfile: %s", err) + return nil, fmt.Errorf("reading Procfile: %s", err) } return pf, nil } diff --git a/start.go b/start.go index 855dfec..70b77ce 100644 --- a/start.go +++ b/start.go @@ -121,13 +121,13 @@ func parseConcurrency(value string) (map[string]int, error) { parts := strings.Split(value, ",") for _, part := range parts { if !strings.Contains(part, "=") { - return concurrency, errors.New("Concurrency should be in the format: foo=1,bar=2") + return concurrency, errors.New("concurrency should be in the format: foo=1,bar=2") } nameValue := strings.Split(part, "=") n, v := strings.TrimSpace(nameValue[0]), strings.TrimSpace(nameValue[1]) if n == "" || v == "" { - return concurrency, errors.New("Concurrency should be in the format: foo=1,bar=2") + return concurrency, errors.New("concurrency should be in the format: foo=1,bar=2") } numProcs, err := strconv.ParseInt(v, 10, 16) From 4d1a6013c44a17c33f5f88dcac03d621656e7f54 Mon Sep 17 00:00:00 2001 From: Tom Wiesing Date: Mon, 7 Jun 2021 13:00:41 -0400 Subject: [PATCH 7/9] procfileEntryRegexp: Use raw string --- procfile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/procfile.go b/procfile.go index 42561c8..2ce90f9 100644 --- a/procfile.go +++ b/procfile.go @@ -9,7 +9,7 @@ import ( "regexp" ) -var procfileEntryRegexp = regexp.MustCompile("^([A-Za-z0-9_-]+):\\s*(.+)$") +var procfileEntryRegexp = regexp.MustCompile(`^([A-Za-z0-9_-]+):\s*(.+)$`) type ProcfileEntry struct { Name string From b125da0e72daa045d5c8be971a0b0ab8054089b2 Mon Sep 17 00:00:00 2001 From: Tom Wiesing Date: Mon, 7 Jun 2021 13:10:59 -0400 Subject: [PATCH 8/9] Revert "Update unix.go (#117)" This reverts commit 51d9f6dc5ffb962e7efe4b461eca53d87587012c. --- unix.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unix.go b/unix.go index c898b7e..9cda033 100644 --- a/unix.go +++ b/unix.go @@ -15,7 +15,7 @@ func ShellInvocationCommand(interactive bool, root, command string) []string { shellArgument = "-ic" } shellCommand := fmt.Sprintf("cd \"%s\"; source .profile 2>/dev/null; exec %s", root, command) - return []string{"sh", shellArgument, shellCommand} + return []string{"bash", shellArgument, shellCommand} } func (p *Process) PlatformSpecificInit() { From 6853b33903b1d05b7331fc61e9189dfe0b362849 Mon Sep 17 00:00:00 2001 From: Tom Wiesing Date: Mon, 7 Jun 2021 13:12:22 -0400 Subject: [PATCH 9/9] Move shell command to variable This commit moves the shell command to be used when executing commands into a variable, allowing it to be customized at build time using a command like: go build -ldflags "-X main.osShell=my-fancy-shell" -o forego --- unix.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/unix.go b/unix.go index 9cda033..6137364 100644 --- a/unix.go +++ b/unix.go @@ -7,6 +7,8 @@ import ( "syscall" ) +var osShell string = "bash" + const osHaveSigTerm = true func ShellInvocationCommand(interactive bool, root, command string) []string { @@ -15,7 +17,7 @@ func ShellInvocationCommand(interactive bool, root, command string) []string { shellArgument = "-ic" } shellCommand := fmt.Sprintf("cd \"%s\"; source .profile 2>/dev/null; exec %s", root, command) - return []string{"bash", shellArgument, shellCommand} + return []string{osShell, shellArgument, shellCommand} } func (p *Process) PlatformSpecificInit() {