Skip to content

Comments

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

Open
MitchLewis930 wants to merge 1 commit intopr_058_beforefrom
pr_058_after
Open

Ensure and enforce that configs are loaded before options are accesse…#8
MitchLewis930 wants to merge 1 commit intopr_058_beforefrom
pr_058_after

Conversation

@MitchLewis930
Copy link

PR_058

@greptile-apps
Copy link

greptile-apps bot commented Jan 30, 2026

Greptile Overview

Greptile Summary

This PR enforces a stricter configuration lifecycle by introducing state guards that ensure clamp is called before accessing configuration options. The main changes include:

  • Configuration lifecycle enforcement: Added @loaded and @clamped flags with a NotClampedError exception that prevents accessing options before clamp is called
  • Simplified API: clamp now calls load internally if needed, providing a single entry point for finalizing configuration
  • Hook refactoring: Changed hook storage from simple blocks/arrays to structured hashes containing {block: ..., cluster_only: ..., id: ...}, enabling better metadata tracking
  • Deferred fork callback warnings: Moved warning logic from DSL (at hook definition time) to Configuration.warn_hooks (at clamp time), ensuring warnings only show after worker count is finalized
  • Reduced coupling: Binder now receives an options hash instead of the full Configuration object, removing unnecessary dependencies

All tests were updated to call clamp before accessing options, and new tests verify the NotClampedError behavior. The changes improve code organization by ensuring configuration is fully resolved before being used.

Confidence Score: 4/5

  • This PR is safe to merge with low risk - it's a well-structured refactoring with comprehensive test coverage
  • The changes follow good software engineering practices by enforcing lifecycle guarantees and reducing coupling. All public API changes are backward-compatible (clamp calls load internally). The comprehensive test updates demonstrate thorough validation. Minor concern: the state machine could have edge cases in multithreaded initialization scenarios, though Puma's usage patterns make this unlikely.
  • lib/puma/configuration.rb should be reviewed carefully to verify the state machine logic is correct

Important Files Changed

Filename Overview
lib/puma/configuration.rb added lifecycle guards (@loaded, @clamped) and enforced clamp before options access, refactored hook storage to use hash structure with metadata
lib/puma/dsl.rb refactored hook processing to store block and metadata in hash format, moved fork callback warning logic from DSL to Configuration
lib/puma/launcher.rb called clamp before accessing options, refactored to pass options hash to Binder instead of Configuration object
lib/puma/binder.rb changed constructor to accept options hash directly instead of Configuration object, removed Configuration dependency
test/test_config.rb updated all tests to call clamp instead of load, added tests for NotClampedError when accessing options before clamp

Sequence Diagram

sequenceDiagram
    participant User
    participant Configuration
    participant DSL
    participant Launcher
    participant Binder

    User->>Configuration: new(user_options, default_options)
    Configuration->>Configuration: Initialize @_options, @loaded=false, @clamped=false
    Configuration->>DSL: new(@_options.user_options)
    Configuration->>DSL: new(@_options.file_options)
    Configuration->>DSL: new(@_options.default_options)
    
    alt User configures via DSL
        User->>DSL: before_fork {block}
        DSL->>Configuration: hooks[:before_fork] = "before_fork"
        DSL->>DSL: Store {block: block, cluster_only: true}
    end
    
    User->>Configuration: clamp()
    Configuration->>Configuration: load() if !@loaded
    Configuration->>Configuration: @loaded = true, @clamped = true
    Configuration->>Configuration: options.finalize_values()
    Configuration->>Configuration: warn_hooks()
    
    alt Worker count is 0 and cluster_only hooks exist
        Configuration->>Configuration: Log warning for cluster_only hooks
    end
    
    User->>Configuration: options
    alt @clamped == false
        Configuration-->>User: raise NotClampedError
    else @clamped == true
        Configuration-->>User: return @_options
    end
    
    User->>Launcher: new(conf, launcher_args)
    Launcher->>Configuration: clamp()
    Launcher->>Configuration: options
    Configuration-->>Launcher: @_options
    Launcher->>Binder: new(log_writer, options)
    Binder->>Binder: Store @options instead of @conf
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

5 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants