Skip to content

Commit 2a344e2

Browse files
rustatianrustatian
authored
chore: simplify, modernize (Go 1.26), update deps (#137)
* chore: simplify, modernize (Go 1.26), update deps - Remove dead `p.log = new(slog.Logger)` alloc (plugin.go) - Extract `prepareCmd` helper — eliminates the three identical command-parse switches in plugin.go / internal.go / init.go - `slices.Clone` replaces manual make+copy for env slices (internal.go) - `strings.Cut` replaces `strings.Split`+len check in initFactory (internal.go) - Merge `case unix: / case tcp:` into `case unix, tcp:` (internal.go) - Concat replaces `fmt.Sprintf` for simple key=value env strings (plugin.go, internal.go, init.go) - `os.ExpandEnv` replaces `os.Expand(v, os.Getenv)` (plugin.go, init.go) - `strconv.Atoi` replaces `strconv.ParseInt(..., 10, 32)` (plugin.go) - Deduplicate `user.Lookup` via new `userInfo()` helper; UID/GID delegate to it (plugin.go) - Fix error log in GID using `cfg.Group` instead of `cfg.User` (plugin.go) - Move `time.NewTimer` after `cmd.Start()` to avoid goroutine leak on Start failure; add `defer timer.Stop()` (init.go) - Fix env ordering in `createProcess`: OS env first, config env appended, matching the main worker path so user config takes precedence over inherited OS env (init.go) - Update tests/ dependencies * fix: use strings.Fields to collapse whitespace in prepareCmd strings.Split on a single space produces empty-string arguments when the command string contains multiple consecutive spaces (e.g. "php app.php" → ["php", "", "app.php"]). strings.Fields trims leading/trailing whitespace and collapses runs of whitespace, matching user intent.
2 parents 88b8b24 + 6742e3b commit 2a344e2

4 files changed

Lines changed: 65 additions & 105 deletions

File tree

‎init.go‎

Lines changed: 12 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package server
22

33
import (
44
"context"
5-
"fmt"
65
"log/slog"
76
"os"
87
"os/exec"
@@ -38,13 +37,15 @@ func (b *command) start() error {
3837
}
3938
}
4039

41-
timer := time.NewTimer(b.cfg.ExecTimeout)
42-
4340
err := cmd.Start()
4441
if err != nil {
4542
return errors.E(op, err)
4643
}
4744

45+
// Start the timer only after the process has launched successfully.
46+
timer := time.NewTimer(b.cfg.ExecTimeout)
47+
defer timer.Stop()
48+
4849
go func() {
4950
errW := cmd.Wait()
5051
if errW != nil {
@@ -71,7 +72,6 @@ func (b *command) start() error {
7172
return nil
7273

7374
case err := <-stopCh:
74-
timer.Stop()
7575
return err
7676
}
7777
}
@@ -83,38 +83,23 @@ func (b *command) Write(data []byte) (int, error) {
8383

8484
// create command for the process
8585
func (b *command) createProcess(env map[string]string, cmd []string) *exec.Cmd {
86-
// cmdArgs contain command arguments if the command in the form of: php <command> or ls <command> -i -b
87-
var cmdArgs []string
88-
var execCmd *exec.Cmd
89-
90-
// TODO!: better way to handle commands, we should not use strings.Split based on space
91-
// here we may have 2 cases: command declared as a space-separated string or as a slice
92-
switch len(cmd) {
93-
// command defined as a space-separated string
94-
case 1:
95-
// we know that the len is 1, so we can safely use the first element
96-
cmdArgs = append(cmdArgs, strings.Split(cmd[0], " ")...)
97-
default:
98-
// we have a slice with 2 or more elements
99-
// first element is the command, the rest are arguments
100-
cmdArgs = cmd
101-
}
86+
cmdArgs := prepareCmd(cmd)
10287

88+
var execCmd *exec.Cmd
10389
if len(cmdArgs) == 1 {
104-
execCmd = exec.CommandContext(context.Background(), cmd[0])
90+
execCmd = exec.CommandContext(context.Background(), cmdArgs[0])
10591
} else {
10692
execCmd = exec.CommandContext(context.Background(), cmdArgs[0], cmdArgs[1:]...)
10793
}
10894

95+
// OS env first, then config env so that user config takes precedence.
96+
execCmd.Env = append(os.Environ(), execCmd.Env...)
97+
10998
// set env variables from the config
110-
if len(env) > 0 {
111-
for k, v := range env {
112-
execCmd.Env = append(execCmd.Env, fmt.Sprintf("%s=%s", strings.ToUpper(k), os.Expand(v, os.Getenv)))
113-
}
99+
for k, v := range env {
100+
execCmd.Env = append(execCmd.Env, strings.ToUpper(k)+"="+os.ExpandEnv(v))
114101
}
115102

116-
// append system envs
117-
execCmd.Env = append(execCmd.Env, os.Environ()...)
118103
// redirect stderr and stdout into the Write function of the process.go
119104
execCmd.Stderr = b
120105
execCmd.Stdout = b

‎internal.go‎

Lines changed: 24 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"log/slog"
77
"os/exec"
8+
"slices"
89
"strings"
910

1011
"github.com/roadrunner-server/errors"
@@ -20,6 +21,16 @@ type internalCommand func() *exec.Cmd
2021
// should be the same as pool.Command
2122
type internalCmdWithArgs func(command []string) *exec.Cmd
2223

24+
// prepareCmd normalises a command slice: a single-element slice whose value is
25+
// a space-separated string is split into individual arguments; a multi-element
26+
// slice is used as-is.
27+
func prepareCmd(command []string) []string {
28+
if len(command) == 1 {
29+
return strings.Fields(command[0])
30+
}
31+
return command
32+
}
33+
2334
// cmdFactory provides worker command factory associated with given context
2435
func (p *Plugin) cmdFactory(env map[string]string) internalCommand {
2536
return func() *exec.Cmd {
@@ -32,14 +43,11 @@ func (p *Plugin) cmdFactory(env map[string]string) internalCommand {
3243
}
3344

3445
// copy prepared envs
35-
cmd.Env = make([]string, len(p.preparedEnvs))
36-
copy(cmd.Env, p.preparedEnvs)
46+
cmd.Env = slices.Clone(p.preparedEnvs)
3747

3848
// append external envs
39-
if len(env) > 0 {
40-
for k, v := range env {
41-
cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", strings.ToUpper(k), v))
42-
}
49+
for k, v := range env {
50+
cmd.Env = append(cmd.Env, strings.ToUpper(k)+"="+v)
4351
}
4452

4553
process.IsolateProcess(cmd)
@@ -56,44 +64,29 @@ func (p *Plugin) cmdFactory(env map[string]string) internalCommand {
5664
}
5765
}
5866

59-
// customCmd used as and enhancement for the CmdFactory to use with a custom command string (used by default)
67+
// customCmd used as an enhancement for the CmdFactory to use with a custom command string (used by default)
6068
func (p *Plugin) customCmd(env map[string]string) internalCmdWithArgs {
6169
return func(command []string) *exec.Cmd {
6270
// if no command provided, use the server's one
6371
if len(command) == 0 {
6472
command = p.cfg.Command
6573
}
6674

67-
var cmd *exec.Cmd
68-
69-
preparedCmd := make([]string, 0, 5)
70-
// here we may have 2 cases: command declared as a space-separated string or as a slice
71-
switch len(command) {
72-
// command defined as a space-separated string
73-
case 1:
74-
// we know that the len is 1, so we can safely use the first element
75-
preparedCmd = append(preparedCmd, strings.Split(command[0], " ")...)
76-
default:
77-
// we have a slice with 2 or more elements
78-
// first element is the command, the rest are arguments
79-
preparedCmd = command
80-
}
75+
preparedCmd := prepareCmd(command)
8176

77+
var cmd *exec.Cmd
8278
if len(preparedCmd) == 1 {
8379
cmd = exec.CommandContext(context.Background(), preparedCmd[0])
8480
} else {
8581
cmd = exec.CommandContext(context.Background(), preparedCmd[0], preparedCmd[1:]...)
8682
}
8783

8884
// copy prepared envs
89-
cmd.Env = make([]string, len(p.preparedEnvs))
90-
copy(cmd.Env, p.preparedEnvs)
85+
cmd.Env = slices.Clone(p.preparedEnvs)
9186

9287
// append external envs
93-
if len(env) > 0 {
94-
for k, v := range env {
95-
cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", strings.ToUpper(k), v))
96-
}
88+
for k, v := range env {
89+
cmd.Env = append(cmd.Env, strings.ToUpper(k)+"="+v)
9790
}
9891

9992
process.IsolateProcess(cmd)
@@ -117,8 +110,8 @@ func initFactory(log *slog.Logger, relay string) (pool.Factory, error) {
117110
return pipe.NewPipeFactory(log), nil
118111
}
119112

120-
dsn := strings.Split(relay, delim)
121-
if len(dsn) != 2 {
113+
network, _, ok := strings.Cut(relay, delim)
114+
if !ok {
122115
return nil, errors.E(op, errors.Network, errors.Str("invalid DSN (tcp://:6001, unix://file.sock)"))
123116
}
124117

@@ -127,11 +120,8 @@ func initFactory(log *slog.Logger, relay string) (pool.Factory, error) {
127120
return nil, errors.E(op, errors.Network, err)
128121
}
129122

130-
switch dsn[0] {
131-
// sockets group
132-
case unix:
133-
return socket.NewSocketServer(lsn, log), nil
134-
case tcp:
123+
switch network {
124+
case unix, tcp:
135125
return socket.NewSocketServer(lsn, log), nil
136126
default:
137127
return nil, errors.E(op, errors.Network, errors.Str("invalid DSN (tcp://:6001, unix://file.sock)"))

‎plugin.go‎

Lines changed: 27 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package server
22

33
import (
44
"context"
5-
"fmt"
65
"log/slog"
76
"os"
87
"os/user"
@@ -52,34 +51,21 @@ func (p *Plugin) Init(cfg Configurer, log NamedLogger) error {
5251
return errors.E(op, errors.Init, err)
5352
}
5453

55-
p.log = new(slog.Logger)
5654
p.log = log.NamedLogger(PluginName)
5755

58-
// here we may have 2 cases: command declared as a space-separated string or as a slice
59-
switch len(p.cfg.Command) {
60-
// command defined as a space-separated string
61-
case 1:
62-
// we know that the len is 1, so we can safely use the first element
63-
p.preparedCmd = append(p.preparedCmd, strings.Split(p.cfg.Command[0], " ")...)
64-
default:
65-
// we have a slice with a 2 or more elements
66-
// first element is the command, the rest are arguments
67-
p.preparedCmd = p.cfg.Command
68-
}
56+
p.preparedCmd = prepareCmd(p.cfg.Command)
6957

70-
p.preparedEnvs = append(os.Environ(), fmt.Sprintf(RrRelay+"=%s", p.cfg.Relay))
58+
p.preparedEnvs = append(os.Environ(), RrRelay+"="+p.cfg.Relay)
7159
if p.rpcCfg != nil && p.rpcCfg.Listen != "" {
72-
p.preparedEnvs = append(p.preparedEnvs, fmt.Sprintf("%s=%s", RrRPC, p.rpcCfg.Listen))
60+
p.preparedEnvs = append(p.preparedEnvs, RrRPC+"="+p.rpcCfg.Listen)
7361
}
7462

7563
// set env variables from the config
76-
if len(p.cfg.Env) > 0 {
77-
for k, v := range p.cfg.Env {
78-
p.preparedEnvs = append(p.preparedEnvs, fmt.Sprintf("%s=%s", strings.ToUpper(k), os.Expand(v, os.Getenv)))
79-
}
64+
for k, v := range p.cfg.Env {
65+
p.preparedEnvs = append(p.preparedEnvs, strings.ToUpper(k)+"="+os.ExpandEnv(v))
8066
}
8167

82-
p.preparedEnvs = append(p.preparedEnvs, fmt.Sprintf("%s=%s", RrVersion, cfg.RRVersion()))
68+
p.preparedEnvs = append(p.preparedEnvs, RrVersion+"="+cfg.RRVersion())
8369

8470
p.factory, err = initFactory(p.log, p.cfg.Relay)
8571
if err != nil {
@@ -160,44 +146,41 @@ func (p *Plugin) NewPoolWithOptions(ctx context.Context, cfg *pool.Config, env m
160146
return pl, nil
161147
}
162148

163-
// UID returns a user id (if specified by user)
164-
func (p *Plugin) UID() int {
149+
// userInfo looks up the user once and returns uid and gid as ints.
150+
func (p *Plugin) userInfo() (uid int, gid int) {
165151
if p.cfg.User == "" {
166-
return 0
152+
return 0, 0
167153
}
168154

169155
usr, err := user.Lookup(p.cfg.User)
170156
if err != nil {
171157
p.log.Error("failed to get user", "id", p.cfg.User)
172-
return 0
158+
return 0, 0
173159
}
174160

175-
usrI32, err := strconv.ParseInt(usr.Uid, 10, 32)
161+
uid, err = strconv.Atoi(usr.Uid)
176162
if err != nil {
177163
p.log.Error("failed to parse user id", "id", p.cfg.User)
178-
return 0
179-
}
180-
181-
return int(usrI32)
182-
}
183-
184-
// GID returns a group id (if specified by user)
185-
func (p *Plugin) GID() int {
186-
if p.cfg.User == "" {
187-
return 0
164+
return 0, 0
188165
}
189166

190-
usr, err := user.Lookup(p.cfg.User)
167+
gid, err = strconv.Atoi(usr.Gid)
191168
if err != nil {
192-
p.log.Error("failed to get user", "id", p.cfg.User)
193-
return 0
169+
p.log.Error("failed to parse group id", "id", p.cfg.User)
170+
return 0, 0
194171
}
195172

196-
grI32, err := strconv.ParseInt(usr.Gid, 10, 32)
197-
if err != nil {
198-
p.log.Error("failed to parse group id", "id", p.cfg.Group)
199-
return 0
200-
}
173+
return uid, gid
174+
}
201175

202-
return int(grI32)
176+
// UID returns a user id (if specified by user)
177+
func (p *Plugin) UID() int {
178+
uid, _ := p.userInfo()
179+
return uid
180+
}
181+
182+
// GID returns a group id (if specified by user)
183+
func (p *Plugin) GID() int {
184+
_, gid := p.userInfo()
185+
return gid
203186
}

‎tests/go.sum‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,9 @@ go.opentelemetry.io/otel v1.44.0/go.mod h1:BMgjTHL9WPRlRjL2oZCBTL4whCGtXch2H4BhO
157157
go.opentelemetry.io/otel/metric v1.44.0 h1:1w0gILTcHdr3YI+ixLyjemwrVnsMURbTZFrSYCdDdmc=
158158
go.opentelemetry.io/otel/metric v1.44.0/go.mod h1:8O7hanEPBNgEMmybD3s2VBKcgWOCsA6tzHBPODAiquo=
159159
go.opentelemetry.io/otel/sdk v1.44.0 h1:nHYwb9lK+fJPU/dnT6s7W7Z8itMWyqrnVfbheVYrZ58=
160+
go.opentelemetry.io/otel/sdk v1.44.0/go.mod h1:Osuydd3Se74nqjAKxid74N5eC+jfEqfTegHRnq58oK0=
160161
go.opentelemetry.io/otel/sdk/metric v1.44.0 h1:3LlKgI+VjbVsjNRFZJZAJ30WjXC5VkNRks6si09iEfI=
162+
go.opentelemetry.io/otel/sdk/metric v1.44.0/go.mod h1:5B5pMARnXxKhltooO4xUuCBorl65a4EpnTalObqOigA=
161163
go.opentelemetry.io/otel/trace v1.44.0 h1:jxF5CsGYCe74MCRx2X4g7WsY/VBKRqqpNvXlX/6gtIk=
162164
go.opentelemetry.io/otel/trace v1.44.0/go.mod h1:oLl1jrMQAVo6v3GAggN+1VH9VIz9iUSvW53sW1Q8PIE=
163165
go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=

0 commit comments

Comments
 (0)