Allow puma config to overwrite threads and workers

[hasghari]

Currently when using the following puma config file, the threads and workers options are ignored because the -w and -t flags appear after the -C flag and will override the workers and threads settings.

#!/usr/bin/env puma

threads ENV.fetch('PUMA_MIN_THREADS', 1), ENV.fetch('PUMA_MAX_THREADS', 1)
worker_boot_timeout ENV.fetch('PUMA_WORKER_BOOT_TIMEOUT', 60)
worker_timeout ENV.fetch('PUMA_WORKER_TIMEOUT', 60)
workers ENV.fetch('PUMA_WORKERS', 1)

The change in this PR allows the puma config file to be authoritative.

[schneems]

I don't think it should matter when in the cli you use the -C option. In general here's how the config works:

1. CLI flags take precedent if provided explicitly.
2. Otherwise values from config/puma.rb take precedent (or another config file you specify)
3. Values or defaults provided from other interfaces such as the Rack handler

That's intentional because it would be really confusing if someone was running:

puma --port 3000

And it booted on a different port.

If you want the values in the config to take precedent, don't provide them via to the puma command.

We could make this more clear perhaps. For example a warning:

Warning: You are specifying `threads` count via `config/puma.rb` and via the flag `-t` 

Though this is the first time i've seen this come up. It's not something I would add at this point, but might take a PR if it wasn't too massive.

[nonrational]

Thanks @schneems. I think the challenge here is that puma-dev controls the invocation of puma and always provides all of those flags.

Upon reflection, I think the best course of action is to conditionally add -w iff $WORKERS is defined, -t iff $THREADS is defined and -C iff $CONFIG is defined. Something like:

WORKERS_ARG=$([ -z "$WORKERS" ] || echo "-w $WORKERS")
THREADS_ARG=$([ -z "$THREADS" ] || echo "-t 0:$THREADS")
CONFIG_ARG=$([ -z "$CONFIG" ] || echo "-C $CONFIG")

puma $WORKERS_ARG $THREADS_ARG $CONFIG_ARG

That means leaning into the puma defaults rather than trying to explicitly set them ourselves.

puma-dev/dev/app.go

Lines 287 to 291 in fca1942

	cmd.Env = append(cmd.Env,
	fmt.Sprintf("THREADS=%d", DefaultThreads),
	"WORKERS=0",
	"CONFIG=-",
	)

Then if you provide all 3 (via .env or similar), you're doing it wrong. @hasghari does that approach seem reasonable to you?

[hasghari]

@nonrational I agree that ideally puma-dev should only supply the workers and threads flags if they have been explicitly defined via environment variables.

[hasghari]

If @schneems agrees with the suggested solution by @nonrational, I can update my pull request with that approach.

[nonrational]

I agree that ideally puma-dev should only supply the workers and threads flags if they have been explicitly defined via environment variables.

Only took 18 months, but I've implemented this approach in #302.

[cjlarose]

@nonrational Let's reopen this until we've got a fix merged.

[nonrational]

I've opened #307 to track this issue.