Skip to content

Ensure and enforce that configs are loaded before options are accesse… #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MitchLewis930 wants to merge 1 commit into
base: pr_058_before
Choose a base branch
from
Open

Ensure and enforce that configs are loaded before options are accesse… #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmarks/local/puma_info.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def initialize(argv, stdout=STDOUT, stderr=STDERR)

if @config_file
config = Puma::Configuration.new({ config_files: [@config_file] }, {})
config.load
config.clamp
@state ||= config.options[:state]
@control_url ||= config.options[:control_url]
@control_auth_token ||= config.options[:control_auth_token]
Expand Down
13 changes: 6 additions & 7 deletions lib/puma/binder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

require_relative 'const'
require_relative 'util'
require_relative 'configuration'

module Puma

Expand All @@ -19,9 +18,9 @@ class Binder

RACK_VERSION = [1,6].freeze

def initialize(log_writer, conf = Configuration.new, env: ENV)
def initialize(log_writer, options, env: ENV)
@log_writer = log_writer
@conf = conf
@options = options
@listeners = []
@inherited_fds = {}
@activated_sockets = {}
Expand All @@ -31,10 +30,10 @@ def initialize(log_writer, conf = Configuration.new, env: ENV)
@proto_env = {
"rack.version".freeze => RACK_VERSION,
"rack.errors".freeze => log_writer.stderr,
"rack.multithread".freeze => conf.options[:max_threads] > 1,
"rack.multiprocess".freeze => conf.options[:workers] >= 1,
"rack.multithread".freeze => options[:max_threads] > 1,
"rack.multiprocess".freeze => options[:workers] >= 1,
"rack.run_once".freeze => false,
RACK_URL_SCHEME => conf.options[:rack_url_scheme],
RACK_URL_SCHEME => options[:rack_url_scheme],
"SCRIPT_NAME".freeze => env['SCRIPT_NAME'] || "",

# I'd like to set a default CONTENT_TYPE here but some things
Expand Down Expand Up @@ -246,7 +245,7 @@ def parse(binds, log_writer = nil, log_msg = 'Listening')
cert_key.each do |v|
if params[v]&.start_with?('store:')
index = Integer(params.delete(v).split('store:').last)
params["#{v}_pem"] = @conf.options[:store][index]
params["#{v}_pem"] = @options[:store][index]
end
end
MiniSSL::ContextBuilder.new(params, @log_writer).context
Expand Down
2 changes: 1 addition & 1 deletion lib/puma/cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ def configure_control_url(command_line_arg)
#

def setup_options(env = ENV)
@conf = Configuration.new({}, {events: @events}, env) do |user_config, file_config|
@conf = Configuration.new({}, { events: @events }, env) do |user_config, file_config|
@parser = OptionParser.new do |o|
o.on "-b", "--bind URI", "URI to bind to (tcp://, unix://, ssl://)" do |arg|
user_config.bind arg
Expand Down
108 changes: 75 additions & 33 deletions lib/puma/configuration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
require_relative 'plugin'
require_relative 'const'
require_relative 'dsl'
require_relative 'events'

module Puma
# A class used for storing "leveled" configuration options.
Expand Down Expand Up @@ -112,7 +113,7 @@ def final_options
# config = Configuration.new({}) do |user_config, file_config, default_config|
# user_config.port 3003
# end
# config.load
# config.clamp
# puts config.options[:port]
# # => 3003
#
Expand All @@ -125,6 +126,9 @@ def final_options
# is done because an environment variable may have been modified while loading
# configuration files.
class Configuration
class NotLoadedError < StandardError; end
class NotClampedError < StandardError; end

DEFAULTS = {
auto_trim_time: 30,
binds: ['tcp://0.0.0.0:9292'.freeze],
Expand Down Expand Up @@ -174,24 +178,35 @@ class Configuration
def initialize(user_options={}, default_options = {}, env = ENV, &block)
default_options = self.puma_default_options(env).merge(default_options)

@options = UserFileDefaultOptions.new(user_options, default_options)
@_options = UserFileDefaultOptions.new(user_options, default_options)
@plugins = PluginLoader.new
@user_dsl = DSL.new(@options.user_options, self)
@file_dsl = DSL.new(@options.file_options, self)
@default_dsl = DSL.new(@options.default_options, self)

if !@options[:prune_bundler]
default_options[:preload_app] = (@options[:workers] > 1) && Puma.forkable?
@events = @_options[:events] || Events.new
@hooks = {}
@user_dsl = DSL.new(@_options.user_options, self)
@file_dsl = DSL.new(@_options.file_options, self)
@default_dsl = DSL.new(@_options.default_options, self)

if !@_options[:prune_bundler]
default_options[:preload_app] = (@_options[:workers] > 1) && Puma.forkable?
end

@puma_bundler_pruned = env.key? 'PUMA_BUNDLER_PRUNED'

if block
configure(&block)
end

@loaded = false
@clamped = false
end

attr_reader :options, :plugins
attr_reader :plugins, :events, :hooks

def options
raise NotClampedError, "ensure clamp is called before accessing options" unless @clamped

@_options
end

def configure
yield @user_dsl, @file_dsl, @default_dsl
Expand All @@ -204,15 +219,15 @@ def configure
def initialize_copy(other)
@conf = nil
@cli_options = nil
@options = @options.dup
@_options = @_options.dup
end
Comment on lines 219 to 223
Copy link

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

State flags not copied in initialize_copy.

The @loaded and @clamped state flags are not copied when duplicating a Configuration object. If a clamped configuration is duplicated, the copy will have @clamped = false (from original initialization), causing NotClampedError when accessing options on the duplicate.

🔧 Proposed fix to copy state flags 
 def initialize_copy(other)
   `@conf`        = nil
   `@cli_options` = nil
   `@_options`     = `@_options.dup`
+  `@loaded`      = other.instance_variable_get(:`@loaded`)
+  `@clamped`     = other.instance_variable_get(:`@clamped`)
 end
🤖 Prompt for AI Agents 
In `@lib/puma/configuration.rb` around lines 219 - 223, When duplicating
Configuration in initialize_copy(other) the state flags `@loaded` and `@clamped` are
not copied, so set them on the new object to match the source; update
initialize_copy to assign `@loaded` = other.instance_variable_get(:`@loaded`) and
`@clamped` = other.instance_variable_get(:`@clamped`) (keep the existing `@_options`
duplication and niling of `@conf/`@cli_options) so the duplicated Configuration
preserves the loaded/clamped state and avoids NotClampedError when calling
options.


def flatten
dup.flatten!
end

def flatten!
@options = @options.flatten
@_options = @_options.flatten
self
end

Expand Down Expand Up @@ -241,28 +256,36 @@ def puma_options_from_env(env = ENV)
end

def load
@loaded = true
config_files.each { |config_file| @file_dsl._load_from(config_file) }

@options
@_options
end
Comment on lines 258 to 262
Copy link

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid marking config as loaded before load completes.
If loading raises, @loaded remains true and prevents retry/guarding, leaving a partially loaded configuration.

✅ Safer load sequencing 
 def load
-  `@loaded` = true
-  config_files.each { |config_file| `@file_dsl._load_from`(config_file) }
-  `@_options`
+  `@loading` = true
+  config_files.each { |config_file| `@file_dsl._load_from`(config_file) }
+  `@loaded` = true
+  `@_options`
+ensure
+  `@loading` = false
 end

 def config_files
-  raise NotLoadedError, "ensure load is called before accessing config_files" unless `@loaded`
+  raise NotLoadedError, "ensure load is called before accessing config_files" unless `@loaded` || `@loading`

Also applies to: 264-276

🤖 Prompt for AI Agents 
In `@lib/puma/configuration.rb` around lines 258 - 262, The load method currently
sets `@loaded` = true before actually processing config_files, so if
`@_file_dsl._load_from` raises the `@loaded` flag stays true; change the sequencing
so `@loaded` is only set after all files are successfully loaded (move the
assignment to after the config_files.each loop) and wrap the load iteration in a
rescue to re-raise after ensuring `@loaded` remains false on error; apply the same
change to the other analogous block covering lines 264-276 that uses
`@file_dsl._load_from` and `@_options` so both code paths only mark loaded after
successful completion.


def config_files
files = @options.all_of(:config_files)
raise NotLoadedError, "ensure load is called before accessing config_files" unless @loaded

files = @_options.all_of(:config_files)

return [] if files == ['-']
return files if files.any?

first_default_file = %W(config/puma/#{@options[:environment]}.rb config/puma.rb).find do |f|
first_default_file = %W(config/puma/#{@_options[:environment]}.rb config/puma.rb).find do |f|
File.exist?(f)
end

[first_default_file]
end

# Call once all configuration (included from rackup files)
# is loaded to flesh out any defaults
# is loaded to finalize defaults and lock in the configuration.
#
# This also calls load if it hasn't been called yet.
def clamp
@options.finalize_values
load unless @loaded
@clamped = true
options.finalize_values
warn_hooks
options
end
Comment on lines 279 to 289
Copy link

@coderabbitai coderabbitai bot Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Set @clamped only after defaults finalize successfully.
If a default proc raises, @clamped stays true and options can be accessed in a partially finalized state.

✅ Suggested sequencing 
 def clamp
   load unless `@loaded`
-  `@clamped` = true
-  options.finalize_values
-  warn_hooks
-  options
+  `@_options.finalize_values`
+  `@clamped` = true
+  warn_hooks
+  `@_options`
 end
🤖 Prompt for AI Agents 
In `@lib/puma/configuration.rb` around lines 279 - 289, clamp currently sets
`@clamped` before finalizing defaults which can leave the object marked clamped if
options.finalize_values raises; fix by deferring setting `@clamped` until after
options.finalize_values (and warn_hooks) succeed: call load unless `@loaded`, run
options.finalize_values and warn_hooks, then set `@clamped` = true and return
options so that `@clamped` is only true on successful finalization (refer to the
clamp method and options.finalize_values/warn_hooks symbols).


# Injects the Configuration object into the env
Expand All @@ -281,11 +304,11 @@ def call(env)
# Indicate if there is a properly configured app
#
def app_configured?
@options[:app] || File.exist?(rackup)
options[:app] || File.exist?(rackup)
end

def rackup
@options[:rackup]
options[:rackup]
end

# Load the specified rackup file, pull options from
Expand All @@ -294,9 +317,9 @@ def rackup
def app
found = options[:app] || load_rackup

if @options[:log_requests]
if options[:log_requests]
require_relative 'commonlogger'
logger = @options[:custom_logger] ? options[:custom_logger] : @options[:logger]
logger = options[:custom_logger] ? options[:custom_logger] : options[:logger]
found = CommonLogger.new(found, logger)
end

Expand All @@ -305,7 +328,7 @@ def app

# Return which environment we're running in
def environment
@options[:environment]
options[:environment]
end

def load_plugin(name)
Expand All @@ -318,13 +341,14 @@ def load_plugin(name)
def run_hooks(key, arg, log_writer, hook_data = nil)
log_writer.debug "Running #{key} hooks"

@options.all_of(key).each do |b|
options.all_of(key).each do |hook_options|
begin
if Array === b
hook_data[b[1]] ||= Hash.new
b[0].call arg, hook_data[b[1]]
block = hook_options[:block]
if id = hook_options[:id]
hook_data[id] ||= Hash.new
block.call arg, hook_data[id]
else
b.call arg
block.call arg
end
rescue => e
log_writer.log "WARNING hook #{key} failed with exception (#{e.class}) #{e.message}"
Expand All @@ -334,7 +358,7 @@ def run_hooks(key, arg, log_writer, hook_data = nil)
end

def final_options
@options.final_options
options.final_options
end

def self.temp_path
Expand All @@ -344,6 +368,12 @@ def self.temp_path
"#{Dir.tmpdir}/puma-status-#{t}-#{$$}"
end

def self.random_token
require 'securerandom' unless defined?(SecureRandom)

SecureRandom.hex(16)
end

private

def require_processor_counter
Expand Down Expand Up @@ -384,22 +414,34 @@ def load_rackup
rack_app, rack_options = rack_builder.parse_file(rackup)
rack_options = rack_options || {}

@options.file_options.merge!(rack_options)
options.file_options.merge!(rack_options)

config_ru_binds = []
rack_options.each do |k, v|
config_ru_binds << v if k.to_s.start_with?("bind")
end

@options.file_options[:binds] = config_ru_binds unless config_ru_binds.empty?
options.file_options[:binds] = config_ru_binds unless config_ru_binds.empty?

rack_app
end

def self.random_token
require 'securerandom' unless defined?(SecureRandom)
def warn_hooks
return if options[:workers] > 0
return if options[:silence_fork_callback_warning]

SecureRandom.hex(16)
@hooks.each do |key, method|
options.all_of(key).each do |hook_options|
next unless hook_options[:cluster_only]

LogWriter.stdio.log(<<~MSG.tr("\n", " "))
Warning: The code in the `#{method}` block will not execute
in the current Puma configuration. The `#{method}` block only
executes in Puma's cluster mode. To fix this, either remove the
`#{method}` call or increase Puma's worker count above zero.
MSG
end
end
end
end
end
3 changes: 2 additions & 1 deletion lib/puma/control_cli.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ def initialize(argv, stdout=STDOUT, stderr=STDERR, env: ENV)
require_relative 'log_writer'

config = Puma::Configuration.new({ config_files: [@config_file] }, {} , env)
config.load
config.clamp

@state ||= config.options[:state]
@control_url ||= config.options[:control_url]
@control_auth_token ||= config.options[:control_auth_token]
Expand Down
Loading