Hanami 2.2 server fails to start with dry-system 1.2 #281

wuarmin · 2025-01-07T16:28:31Z

Describe the bug

If I start the hanami server I get following backtrace:

! Unable to load application: ArgumentError: wrong number of arguments (given 0, expected 1+)
/usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/config.rb:112 class_eval:2:in `__dry_initializer_initialize__': wrong number of arguments (given 0, expected 1+) (ArgumentError)
        from /usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/mixin/root.rb:11:in `initialize'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:68:in `block (2 levels) in define_initialize_with_splat'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `new'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `block (2 levels) in define_new'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/loader.rb:54:in `call'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/component.rb:64:in `instance'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/auto_registrar.rb:34:in `block (2 levels) in call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/item/callable.rb:16:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/resolver.rb:36:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/mixin.rb:132:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:493:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `block (2 levels) in finalize!'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `each'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `block in finalize!'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:665:in `run_hooks'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:322:in `finalize!'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice.rb:330:in `boot'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice_registrar.rb:64:in `each_value'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice_registrar.rb:64:in `each'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice.rb:331:in `boot'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami.rb:214:in `boot'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/boot.rb:5:in `<top (required)>'
        from /usr/local/lib/ruby/3.3.0/bundled_gems.rb:75:in `require'
        from /usr/local/lib/ruby/3.3.0/bundled_gems.rb:75:in `block (2 levels) in replace_require'
        from config.ru:3:in `block in <main>'
        from /usr/local/bundle/gems/rack-2.2.10/lib/rack/builder.rb:116:in `eval'
        from /usr/local/bundle/gems/rack-2.2.10/lib/rack/builder.rb:116:in `new_from_string'
        from /usr/local/bundle/gems/rack-2.2.10/lib/rack/builder.rb:105:in `load_file'
        from /usr/local/bundle/gems/rack-2.2.10/lib/rack/builder.rb:66:in `parse_file'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/configuration.rb:384:in `load_rackup'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/configuration.rb:297:in `app'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/runner.rb:166:in `load_and_bind'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/single.rb:44:in `run'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/launcher.rb:196:in `run'
        from /usr/local/bundle/gems/puma-6.5.0/lib/puma/cli.rb:75:in `run'
        from /usr/local/bundle/gems/puma-6.5.0/bin/puma:10:in `<top (required)>'
        from /usr/local/bundle/bin/puma:25:in `load'
        from /usr/local/bundle/bin/puma:25:in `<main>'

To Reproduce

calling bundle update at a Hanami 2.2 project updates dry-system to 1.2 because of
https://github.com/hanami/hanami/blob/2ca35811d4f2c7b0535c5bffdd973db9327f92ca/hanami.gemspec#L39

Expected behavior

The hanami server should start with no errors.

My environment

Affects my production application: NO
Ruby version: 3.3.5
OS: Debian bookworm

The text was updated successfully, but these errors were encountered:

flash-gordon · 2025-01-07T16:33:07Z

@wuarmin thanks for checking it out. Do you know how to reproduce it?

cllns · 2025-01-07T19:13:07Z

I just tried to reproduce this in my application and I wasn't able to. I first updated dry-system to 1.2 and then also updated dry-initializer to 3.2. All my tests pass and I'm able to start the server manually as well. I then updated all my gems and it's still working for me.

I really want to make sure others don't run into this problem, so any tips on reproducing would be super helpful.

wuarmin · 2025-01-08T09:39:09Z

Hey @cllns,
it has something to do with auto-registration. In my opinion, the boot behavior has changed in dry-system. Let me explain:

I have custom-changesets like:

# ots/repos/changesets/new_object_changeset.rb

module Ots
  module Repos
    module Changesets
      class NewObjectChangeset < ROM::Changeset::Create
        include Deps["now"]

        map do |tuple|
          current_time = now.call
          tuple.except(:id).merge(created_at: current_time, updated_at: current_time)
        end
      end
    end
  end
end

Actually, “auto_register: false” would be appropriate here , but I forgot.

Behavior with dry-system 1.1:

services[development]> Ots::Slice.boot
=> Ots::Slice

With dry-system 1.1 no error happens at boot-time. To provoke the error, I have to explicitly access the key:

services[development]> Ots::Slice.boot
=> Ots::Slice
services[development]> Ots::Slice["repos.changesets.new_object_changeset"]
/usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/config.rb:112 class_eval:2:in `__dry_initializer_initialize__': wrong number of arguments (given 0, expected 1+) (ArgumentError)
        from /usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/mixin/root.rb:11:in `initialize'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:68:in `block (2 levels) in define_initialize_with_splat'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `new'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `block (2 levels) in define_new'
        from /usr/local/bundle/gems/dry-system-1.1.1/lib/dry/system/loader.rb:54:in `call'
        from /usr/local/bundle/gems/dry-system-1.1.1/lib/dry/system/component.rb:64:in `instance'
        from /usr/local/bundle/gems/dry-system-1.1.1/lib/dry/system/auto_registrar.rb:34:in `block (2 levels) in call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/item/callable.rb:16:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/resolver.rb:36:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/mixin.rb:132:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.1.1/lib/dry/system/container.rb:493:in `resolve'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/mixin.rb:145:in `[]'
        from /usr/local/bundle/gems/hanami-2.2.1/lib/hanami/slice.rb:624:in `[]'
        from (irb):2:in `<main>'
        from <internal:kernel>:187:in `loop'
        from /usr/local/bundle/gems/hanami-cli-2.2.1/lib/hanami/cli/commands/app/console.rb:48:in `call'
        ... 4 levels...

Behavior with dry-system 1.2

services[development]> Ots::Slice.boot
/usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/config.rb:112 class_eval:2:in `__dry_initializer_initialize__': wrong number of arguments (given 0, expected 1+) (ArgumentError)
        from /usr/local/bundle/gems/dry-initializer-3.2.0/lib/dry/initializer/mixin/root.rb:11:in `initialize'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:68:in `block (2 levels) in define_initialize_with_splat'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `new'
        from /usr/local/bundle/gems/dry-auto_inject-1.1.0/lib/dry/auto_inject/strategies/kwargs.rb:19:in `block (2 levels) in define_new'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/loader.rb:57:in `call'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/component.rb:64:in `instance'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/auto_registrar.rb:34:in `block (2 levels) in call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/item/callable.rb:16:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/resolver.rb:36:in `call'
        from /usr/local/bundle/gems/dry-core-1.1.0/lib/dry/core/container/mixin.rb:132:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:493:in `resolve'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `block (2 levels) in finalize!'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `each'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:327:in `block in finalize!'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:665:in `run_hooks'
        from /usr/local/bundle/gems/dry-system-1.2.0/lib/dry/system/container.rb:322:in `finalize!'

Here the error happens at boot-time.

Conclusion:

With dry-system 1.1 there is no component-instantiation during the boot process, which in my opinion is correct.
With dry-system 1.2 there is a component-instantiation during boot process, which is unnecessary as you will receive a new instantiation when you access it anyway (except the component should be memoized).

What do you think?

Update
I think it is because of the eager_load-ft #276

Here is a quote from you:

Running .finalize! on a container would check to see if constants that correspond to each key that's registered are defined. It shouldn't actually resolve the components, but it should check to see if the components are resolveable (preresolve?)

In my opinion, it meets your requirement exactly. We want to see if the constant is defined and not create it completely. Or are you of the opinion that the extra instancing doesn't matter? I just think that users might be confused if the objects are created right at the start. It could lead to concerns that the object is not always recreated.

best regards

timriley · 2025-01-08T11:21:06Z

That's some great investigation, thank you @wuarmin!

It looks to me like the behaviour added via #276 has actually exposed an issue in your app setup: that your changeset classes should have been opted out from auto-registration, either via # auto_register: false magic comments or via container config (which you can do through Hanami with the app's config.no_auto_register_paths). Is this right?

So if you made these changes, @wuarmin, would your app boot again?

The issue you've hit here also makes me think we should add a conventional changesets path to our default value for no_auto_register_paths. We don't want to create extra work for users when they start using changesets. (n.b. I would not set it to be repos/changesets as you have used, @wuarmin, I would have it instead be db/changesets, since db/ is the directory we create for users to place their internal ROM components).

Now as for the change in #276, I do have to apologise: part of why I never commented on that PR is because I didn't want to bring this behaviour for Hanami 2.2 without due consideration, especially since that change was a "nice to have" and not essential for the Hanami release. I realise I should have been clear from the start and written that in a comment on the PR. I'm sorry!

This dry-system change relates to a few other areas we still need to investigate:

The impact of defaulting to memoize: true for components in Hanami apps
Whether we should set eager_load: true for dry-system's Zeitwerk plugin when hanami is in production mode (right now we have it as always false)
And measuring the performance impact of all of these

IMO, these are the kinds of things that would require some thorough testing and would eventually become part of a significant new Hanami release, e.g. 2.3 or later.

Given #276 feature intersects with all of this, and given dry-system 1.2.0 has only been out for a day, I'd be inclined to revert #276 for now, and roll forward to a dry-system 1.2.1 release with that feature removed again.

@cllns @flash-gordon — would you be OK if we did this? Do you have any alternative approaches you'd rather we try?

timriley · 2025-01-08T11:27:46Z

One last thing: looking at the CHANGELOG line for that dry-system feature:

Option to skip eager loading during finalize with eager_load: false

...I wonder if @flash-gordon considered this change to be more minor than it actually was? It does add an eager_load: option, yes, but it also adds the whole eager loading machinery at the same time, which is a high-impact change that probably could have done with some more team discussion before we merged.

This is OK, though! It's just a learning experience for us :) dry-system is one of our more critical gems, and going forward we probably need to put a bit more time into assessing changes there before release.

flash-gordon · 2025-01-08T11:28:27Z

@timriley That's actually my dyslexia. I somehow read eager_load: true as false; otherwise, I would be a lot more cautious with this change. Maybe we can just set it to false for now and push 1.2.1?

flash-gordon · 2025-01-08T11:29:40Z

Also, maybe more testing is needed for this

timriley · 2025-01-08T11:34:42Z

@flash-gordon all good :) And thanks for hopping back into this thread so quickly! ❤️

Since the change is so small, and since it's still an open question about how exactly we address the fundamental issue ("how can we surface name errors in user code earlier?"), I think it's worth just reverting this change and pushing that out as 1.2.1.

This would give us the time and space to consider some different options.

What do you think?

First of all, this is a breaking change that I failed to foresee. We can discuss re-adding it in a less impactful manner, e.g. setting it to false by default. For now I just toll it back.

First, this is a breaking change that I failed to foresee. We can discuss re-adding it in a less impactful manner, e.g. setting it to false by default. For now I just roll it back.

flash-gordon · 2025-01-08T11:50:35Z

@timriley I don't mind reverting #282
Interestingly, it didn't impact me because I intentionally check all components are loadable in dev/prod environments

flash-gordon · 2025-01-08T11:57:08Z

1.2.1 is out. @wuarmin please check if it works for you

wuarmin · 2025-01-08T12:04:56Z

@timriley thank you

So if you made these changes, @wuarmin, would your app boot again?

Yes, with # auto_register: false magic command at the changesets, my app boots again.

@flash-gordon thank you, the behavior is the same again.

One more question: Is it really planned to memoize Hanami components by default? One impact that I can well imagine here is that not all users implement their components thread-safe.

What do you think?

timriley · 2025-01-08T12:23:16Z

Thanks @flash-gordon!

I intentionally check all components are loadable in dev/prod environments

I'd be very interested to see how you handle this in your apps, if it's something you can share :)

So if you made these changes, @wuarmin, would your app boot again?

Yes, with # auto_register: false magic command at the changesets, my app boots again.

@wuarmin this is good to know! I'd recommend you keep those comments in place or add that path to config.no_auto_register_paths to ensure that you're not caught out by changes like this, which still may be reintroduced in the future.

One more question: Is it really planned to memoize Hanami components by default? One impact that I can well imagine here is that not all users implement their components thread-safe.

Yes, we'll need to consider threaded webservers when we do this. A reasonable starting point could be to memoize the components that we can trust to behave reasonably, such as anything that inherits from Hanami-provided classes, like actions, views, repos, relations, etc.

Anyway, by the time we get to a change like this, we'll be certain to communicate clearly to the community about it and make sure we publish some prereleases so that the community can help test and verify their apps.

flash-gordon · 2025-01-08T13:16:59Z

I'd be very interested to see how you handle this in your apps, if it's something you can share :)

@timriley took me a minute to find but it's here: https://github.com/dry-rb/dry-effects/blob/8f8f1488fcac976e05fc0075a3cc9d476e903761/lib/dry/effects/extensions/system.rb#L38

cllns · 2025-01-08T22:11:57Z

Ah, sorry everyone for my PR #276 causing this. I should've dug deeper in the stack trace to see the error came from a change in that PR (dry-system-1.2.0/lib/dry/system/container.rb:327) and figured this out. Thanks for digging into it. Regardless of the issue, I think this was a fruitful discussion, in terms of:

adding db/changesets (and db/commands?) to no_auto_register_paths
standards for memoizing components & thread-safety
adding scrutiny for merging PR's to dry-system

This reverts commit 7b8ae92.

flash-gordon · 2025-01-09T11:57:41Z

@cllns @timriley I re-opened PR for further discussions #283

wuarmin added bug help wanted labels Jan 7, 2025

wuarmin changed the title ~~Hanami fails to start with dry-system 1.2~~ Hanami 2.2 server fails to start with dry-system 1.2 Jan 7, 2025

flash-gordon mentioned this issue Jan 8, 2025

Revert eager_load option in Container#finalize! #282

Merged

flash-gordon closed this as completed in 7b8ae92 Jan 8, 2025

flash-gordon added a commit that referenced this issue Jan 9, 2025

Revert "Revert eager_load option in Container#finalize! (fix #281)"

6b26b02

This reverts commit 7b8ae92.

flash-gordon added a commit that referenced this issue Jan 9, 2025

Revert "Revert eager_load option in Container#finalize! (fix #281)"

b96e64c

This reverts commit 7b8ae92.

flash-gordon mentioned this issue Jan 9, 2025

Adding back eager_load: option in Container#finalize!" #283

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hanami 2.2 server fails to start with dry-system 1.2 #281

Hanami 2.2 server fails to start with dry-system 1.2 #281

wuarmin commented Jan 7, 2025 •

edited

Loading

flash-gordon commented Jan 7, 2025

cllns commented Jan 7, 2025

wuarmin commented Jan 8, 2025 •

edited

Loading

timriley commented Jan 8, 2025 •

edited

Loading

timriley commented Jan 8, 2025 •

edited

Loading

flash-gordon commented Jan 8, 2025

flash-gordon commented Jan 8, 2025

timriley commented Jan 8, 2025 •

edited

Loading

flash-gordon commented Jan 8, 2025

flash-gordon commented Jan 8, 2025

wuarmin commented Jan 8, 2025 •

edited

Loading

timriley commented Jan 8, 2025 •

edited

Loading

flash-gordon commented Jan 8, 2025 •

edited

Loading

cllns commented Jan 8, 2025

flash-gordon commented Jan 9, 2025

Hanami 2.2 server fails to start with dry-system 1.2 #281

Hanami 2.2 server fails to start with dry-system 1.2 #281

Comments

wuarmin commented Jan 7, 2025 • edited Loading

Describe the bug

To Reproduce

Expected behavior

My environment

flash-gordon commented Jan 7, 2025

cllns commented Jan 7, 2025

wuarmin commented Jan 8, 2025 • edited Loading

Behavior with dry-system 1.1:

Behavior with dry-system 1.2

timriley commented Jan 8, 2025 • edited Loading

timriley commented Jan 8, 2025 • edited Loading

flash-gordon commented Jan 8, 2025

flash-gordon commented Jan 8, 2025

timriley commented Jan 8, 2025 • edited Loading

flash-gordon commented Jan 8, 2025

flash-gordon commented Jan 8, 2025

wuarmin commented Jan 8, 2025 • edited Loading

timriley commented Jan 8, 2025 • edited Loading

flash-gordon commented Jan 8, 2025 • edited Loading

cllns commented Jan 8, 2025

flash-gordon commented Jan 9, 2025

wuarmin commented Jan 7, 2025 •

edited

Loading

wuarmin commented Jan 8, 2025 •

edited

Loading

timriley commented Jan 8, 2025 •

edited

Loading

timriley commented Jan 8, 2025 •

edited

Loading

timriley commented Jan 8, 2025 •

edited

Loading

wuarmin commented Jan 8, 2025 •

edited

Loading

timriley commented Jan 8, 2025 •

edited

Loading

flash-gordon commented Jan 8, 2025 •

edited

Loading