ddollar / foreman

Manage Procfile-based applications
http://ddollar.github.com/foreman
MIT License
6.01k stars 630 forks source link

[engine/cli.rb] Handle nil name_padding #769

Closed NickLaMuro closed 4 months ago

NickLaMuro commented 4 years ago

When (mistakenly) starting foreman with a empty Procfile, it ends up not being possible to exit this process via a TERM signal due to the following error:

$ foreman start
ERROR: Procfile does not exist.
$ touch Procfile
$ bin/foreman start
^Ccomparison of NilClass with 6 failed
./lib/foreman/engine/cli.rb:80:in `name_padding'
./lib/foreman/engine/cli.rb:85:in `pad_process_name'
./lib/foreman/engine/cli.rb:61:in `block in output'
./lib/foreman/engine/cli.rb:57:in `each'
./lib/foreman/engine/cli.rb:57:in `output'
./lib/foreman/engine.rb:335:in `block in output_with_mutex'
./lib/foreman/engine.rb:334:in `synchronize'
./lib/foreman/engine.rb:334:in `output_with_mutex'
./lib/foreman/engine.rb:340:in `system'
./lib/foreman/engine.rb:124:in `handle_interrupt'
./lib/foreman/engine.rb:104:in `handle_signal'
./lib/foreman/engine.rb:389:in `handle_signals'
./lib/foreman/engine.rb:412:in `block (2 levels) in watch_for_output'
./lib/foreman/engine.rb:409:in `loop'
./lib/foreman/engine.rb:409:in `block in watch_for_output'
^C^C^C^C^C^X^Z
[1]+  Stopped                 bin/foreman start
$ kill %1
$ jobs
[1]+  Running                 bin/foreman start &
$ jobs
[1]+  Running                 bin/foreman start &
$ kill -9 %1
[1]+  Killed: 9               bin/foreman start
$ jobs

By adding a .to_i, this simply allows the Engine::Cli#name_padding method to default to 6, allowing the rest of the shutdown process to finish executing and exit the process gracefully.

Alternatively, this could be fixed by not allowing an empty Procfile to be valid (was actually not addressed in https://github.com/ddollar/foreman/issues/620 ), which would most likely prevent this state from being feasible, but I think having this catch as well doesn't hurt anything. I will look into creating a PR to also solve for this as well.

ddollar commented 4 months ago

Looks like a good check, should also be covered by #770

Thank you!