Incompatibility with kt-paperclip #306

jwoodrow · 2023-01-10T17:02:37Z

Hi, first off good work on YJIT.

I've been trying to upgrade to ruby 3.2 with YJIT enabled and noticed some issues with the kt-paperclip when used with rails.

We have an old model we haven't attempted to migrate to active_storage because of the sheer size of this model and so we have kt-paperclip still in the project for this. In our demo generation suite (using factorybot) we end up using the model_instance.attachment.assign method which then calls some validations but doing so seems to completely break when using yjit.

Here is the error and the stack trace if it can help:

NoMethodError: undefined method `_read_attribute' for :read_attribute_for_validation:Symbol
/home/docker/bundle/vendor/ruby/3.2.0/gems/activemodel-7.0.4/lib/active_model/attribute_methods.rb:277:in `attachment_file_name'
/home/docker/bundle/vendor/ruby/3.2.0/gems/kt-paperclip-7.1.1/lib/paperclip/validators/attachment_file_name_validator.rb:19:in `validate_each'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activemodel-7.0.4/lib/active_model/validator.rb:153:in `block in validate'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activemodel-7.0.4/lib/active_model/validator.rb:149:in `each'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activemodel-7.0.4/lib/active_model/validator.rb:149:in `validate'
/home/docker/bundle/vendor/ruby/3.2.0/gems/kt-paperclip-7.1.1/lib/paperclip/validators.rb:68:in `block in create_validating_before_filter'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activesupport-7.0.4/lib/active_support/callbacks.rb:445:in `instance_exec'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activesupport-7.0.4/lib/active_support/callbacks.rb:445:in `block in make_lambda'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activesupport-7.0.4/lib/active_support/callbacks.rb:199:in `block (2 levels) in halting'
/home/docker/bundle/vendor/ruby/3.2.0/gems/kt-paperclip-7.1.1/lib/paperclip/callbacks.rb:28:in `block in hasta_la_vista_baby'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activesupport-7.0.4/lib/active_support/callbacks.rb:200:in `block in halting'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activesupport-7.0.4/lib/active_support/callbacks.rb:595:in `block in invoke_before'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activesupport-7.0.4/lib/active_support/callbacks.rb:595:in `each'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activesupport-7.0.4/lib/active_support/callbacks.rb:595:in `invoke_before'
/home/docker/bundle/vendor/ruby/3.2.0/gems/activesupport-7.0.4/lib/active_support/callbacks.rb:106:in `run_callbacks'
/home/docker/bundle/vendor/ruby/3.2.0/gems/kt-paperclip-7.1.1/lib/paperclip/callbacks.rb:38:in `run_paperclip_callbacks'
/home/docker/bundle/vendor/ruby/3.2.0/gems/kt-paperclip-7.1.1/lib/paperclip/attachment.rb:504:in `check_validity?'
/home/docker/bundle/vendor/ruby/3.2.0/gems/kt-paperclip-7.1.1/lib/paperclip/attachment.rb:494:in `post_process'
/home/docker/bundle/vendor/ruby/3.2.0/gems/kt-paperclip-7.1.1/lib/paperclip/attachment.rb:454:in `post_process_file'

I know this is linked to YJIT because if I remove the RUBYOPT="--enable-yjit" before running the demo generation then everything works fine.

I don't understand exactly why this is happening, but I'm guessing/hoping I'm not the only one encountering issues using YJIT + kt-paperclip.

Maybe this is an issue I should raise on the kt-paperclip repo but I have a feeling it's more like some missing implementations in YJIT than kt-paperclip being broken (since it still works fine with the default cruby). Any insight into resolving this would be welcome ! 👋

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

maximecb · 2023-01-10T18:50:29Z

Hi @jwoodrow,

Can you provide more context in terms of what machine you are running this on (Mac/Linux distro, are you on an x86 or ARM machine?).

Would also like to know if you can provide a minimal script that reproduces the error and which command you've used to run it. I've never heart of kt-paperclip or factorybot before, and I don't know which codebase you're running this on, so I wouldn't know how to reproduce this issue.

jwoodrow · 2023-01-10T18:57:23Z

Hi @maximecb, I'm currently away from my work computer so I can't provide a reproduction context, but I can try and work on that starting tomorrow.

For the other questions, I'm running in a x86 ubuntu docker image and kt-paperclip is the replacing gem for paperclip which was the predecessor of active_storage before it came to be. The error itself appears during the validation process it seems. My first instinct is that there seems to be some issues with how Symbols are managed by unit because I've also encountered another similar issue where a gem used for json validations said there was no gsub method defined for a given symbol 🤔

I'll try and get you a reproduction repo up and running when I can (probably using docker so you can reproduce it more consistently)

maximecb · 2023-01-10T19:16:18Z

If you're able to provide us with the smallest possible script that reproduces the problem, that will help us a lot in diagnosing and fixing the issue. If there's something wrong with the way we handle symbols, that is definitely something we want to fix.

jwoodrow · 2023-01-11T10:54:00Z

I've been trying to reproduce using a newly created docker container and a minimal setup but it doesn't seem to be producing the error we're encountering on our main codebase. Which makes me thing, maybe we've got something else in our code which might be causing this issue, but with the size of it I think this might be a hard issue to debug for us so we might have to make a pass on yjit unfortunately for us 😞

maximecb · 2023-01-11T15:07:36Z

Ok, well, if there is an issue with symbols, it will probably get fixed eventually when we can get a repro from another source :)

geoffharcourt · 2023-01-19T14:11:59Z

Possibly related: we had something happen to us inconsistently but frequently in CI that went away as soon as we disabled YJIT. We haven't seen this in production and haven't been able to reproduce it on our development machines:

kreeti/kt-paperclip#107

XrXr · 2023-01-19T14:22:21Z

I suggest trying a build of YJIT enabled Ruby off of the master branch. We've landed a couple of bug fixes since the 3.0 release that haven't been released yet. You can follow instructions to build from a checkout, or use https://github.com/rbenv/ruby-build with a checkout or a nightly snapshot release tar ball. There are also nightly builds of container images available at https://hub.docker.com/r/rubylang/ruby.

maximecb · 2023-01-19T15:15:13Z

@geoffharcourt if you have the time, checking if this bug is fixed by building ruby master as Alan suggested would be very helpful.

johnnyshields · 2023-01-29T18:37:29Z

FYI -- at TableCheck we're running kt-paperclip and YJIT in production and not seeing any issues so far.

maximecb · 2023-01-30T16:11:35Z

FYI -- at TableCheck we're running kt-paperclip and YJIT in production and not seeing any issues so far.

Thanks for the feedback @johnnyshields. Also very happy to hear that you are running YJIT in production :)

Will leave this issue open for a little bit longer, just in case anyone else has anything to report.

geoffharcourt · 2023-01-30T18:31:09Z

Apologies for not running the test sooner, our CI system isn't set up to run Ruby from the master branch and we've only been able to produce this issue so far in CI.

josh-m-sharpe · 2023-02-09T21:32:42Z

Hi all 👋 - I can reproduce this (or very similar) issue in our local test suite (rails 6.1.x / ruby 3.2.1 yjit / kt-paperclip). Interestingly it only fails if I run the suite or multiple tests files/methods at once. It doesn't fail if I run the single test method (e.g. rails test path/to/test.rb:134)

@maximecb can you confirm if the fixes you speak of are a part of 3.2.1 or should I attempt with master?

(Also hey there @geoffharcourt - long time. Hope you're well!)

maximecb · 2023-02-09T22:04:24Z

We've merged multiple bug fixes as part of 3.2.1. You could start with that one. Let us know if that fixes the issue or not.

If neither 3.2.1 or master fixes the issue, we could potentially pair with you to try and diagnose the problem.

josh-m-sharpe · 2023-02-09T22:09:23Z

I’ve seen it fail with both 3.2.0 and 3.2.1.

I’m open to pairing. I usually work EST 9-5. Please let me know how best we can schedule a time. Thanks!

maximecb · 2023-02-10T19:35:17Z

Tentatively Tuesday, Wednesday or Thursday in the afternoon would work best for us. Would that work for you as well?

We typically use Tuple for pairing, but we could use another solution if you prefer another platform. You'll need to have the rust toolchain and lldb or gdb installed.

Previously, YJIT failed to put the stack into the correct shape when `BasicObject#send` calls an alias method for the send method itself. This can manifest as strange `NoMethodError`s in the final non-send receiver, as [seen][1] with the kt-paperclip gem. I also found a case where it makes YJIT fail the stack size assertion while compiling `leave`. YJIT's `BasicObject#send` implementation already rejects sends to `send`, but didn't detect sends to aliases of `send`. Adjust the detection and reject these cases. Fixes [Bug #19464] [1]: Shopify/yjit#306

Previously, YJIT failed to put the stack into the correct shape when `BasicObject#send` calls an alias method for the send method itself. This can manifest as strange `NoMethodError`s in the final non-send receiver, as [seen][1] with the kt-paperclip gem. I also found a case where it makes YJIT fail the stack size assertion while compiling `leave`. YJIT's `BasicObject#__send__` implementation already rejects sends to `send`, but didn't detect sends to aliases of `send`. Adjust the detection and reject these cases. Fixes [Bug #19464] [1]: Shopify/yjit#306

maximecb · 2023-02-27T16:13:44Z

Hi there! Alan just merged a PR which he believes may fix this problem.

If someone could test whether the bug is fixed by commit 0eb634a, that would be very helpful.

Have a great week everyone :)

XrXr · 2023-02-27T23:45:35Z

The commit landed patches 3.2.1 cleanly, so you can try it without making your app compatible with the latest development version of Ruby. Here is how to build with the patch (the build takes less than 10 minutes!):

If you use ruby-install (I tried on an ARM64 Mac):

ruby-install ruby 3.2.1 \
                      --jobs 8 \
                      --no-reinstall \
                      --install-dir ~/.rubies/ruby-yjit-send-fix \
                      --patch https://github.com/ruby/ruby/commit/0eb634ae73cb327ede833b72492f912792a4a9d5.patch \
                      -- --enable-yjit

If you use rbenv with ruby-build (I tried on x86_64-linux-gnu):

# make sure `rustc` is available and works
rbenv install --patch 3.2.1 \
	< <(curl --proto '=https' --tlsv1.2 -sSf https://github.com/ruby/ruby/commit/0eb634ae73cb327ede833b72492f912792a4a9d5.patch)

Depending on your setup, you might not even need to reinstall the bundle for your Rails app when switching to the patched version.

Let us know how it goes if you do decide to give this a go! It'd be valuable feedback for us and for the release manager for the 3.2.x series. You can use the repro script from https://bugs.ruby-lang.org/issues/19464 for a quick smoke test to see if the patch applied properly.

josh-m-sharpe · 2023-02-28T01:45:07Z

@maximecb @XrXr I managed to get ruby built from master downloaded here https://github.com/ruby/ruby - which seems to resolve this issue - at least, our test suite has passed.

Any guess as to when 3.2.2 might drop? It'll be much easier to further test this patch with a released version.

Thank you!

XrXr · 2023-02-28T15:08:51Z

Any guess as to when 3.2.2 might drop?

Generally, the branch maintainer decides the timing and the contents of releases. I think @nurse is the current maintainer of the ruby_3_2 branch. Historically, there are gaps measured in weeks between releasees, but past results do not necessarily predict the future.

Thank you!

Thank you for testing!

YJIT: Detect and reject `send(:alias_for_send, :foo)` Previously, YJIT failed to put the stack into the correct shape when `BasicObject#send` calls an alias method for the send method itself. This can manifest as strange `NoMethodError`s in the final non-send receiver, as [seen][1] with the kt-paperclip gem. I also found a case where it makes YJIT fail the stack size assertion while compiling `leave`. YJIT's `BasicObject#__send__` implementation already rejects sends to `send`, but didn't detect sends to aliases of `send`. Adjust the detection and reject these cases. Fixes [Bug #19464] [1]: Shopify/yjit#306 --- test/ruby/test_yjit.rb | 20 ++++++++++++++++++++ yjit/src/codegen.rs | 25 ++++++++++--------------- 2 files changed, 30 insertions(+), 15 deletions(-)

gregpardo mentioned this issue Feb 3, 2023

Update tests for Ruby 3 compatibilty and fix YJIT read_attribute_for_validation kreeti/kt-paperclip#109

Closed

XrXr mentioned this issue Feb 25, 2023

YJIT: 2 __send__ miscompilations ruby/ruby#7377

Merged

1 task

untitaker mentioned this issue Feb 27, 2023

Ruby 3.2.1 SideKiq error - LinkCrawlWorker mastodon/mastodon#23637

Closed

XrXr closed this as completed Feb 28, 2023

This was referenced Mar 13, 2023

Support Ruby 3.1 mastodon/mastodon#20333

Closed

Enabel YJIT by default mastodon/mastodon#24091

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incompatibility with kt-paperclip #306

Incompatibility with kt-paperclip #306

jwoodrow commented Jan 10, 2023

maximecb commented Jan 10, 2023

jwoodrow commented Jan 10, 2023

maximecb commented Jan 10, 2023

jwoodrow commented Jan 11, 2023

maximecb commented Jan 11, 2023

geoffharcourt commented Jan 19, 2023

XrXr commented Jan 19, 2023 •

edited

Loading

maximecb commented Jan 19, 2023

johnnyshields commented Jan 29, 2023 •

edited

Loading

maximecb commented Jan 30, 2023

geoffharcourt commented Jan 30, 2023

josh-m-sharpe commented Feb 9, 2023

maximecb commented Feb 9, 2023

josh-m-sharpe commented Feb 9, 2023

maximecb commented Feb 10, 2023

maximecb commented Feb 27, 2023 •

edited

Loading

XrXr commented Feb 27, 2023 •

edited

Loading

josh-m-sharpe commented Feb 28, 2023 •

edited

Loading

XrXr commented Feb 28, 2023

Incompatibility with kt-paperclip #306

Incompatibility with kt-paperclip #306

Comments

jwoodrow commented Jan 10, 2023

maximecb commented Jan 10, 2023

jwoodrow commented Jan 10, 2023

maximecb commented Jan 10, 2023

jwoodrow commented Jan 11, 2023

maximecb commented Jan 11, 2023

geoffharcourt commented Jan 19, 2023

XrXr commented Jan 19, 2023 • edited Loading

maximecb commented Jan 19, 2023

johnnyshields commented Jan 29, 2023 • edited Loading

maximecb commented Jan 30, 2023

geoffharcourt commented Jan 30, 2023

josh-m-sharpe commented Feb 9, 2023

maximecb commented Feb 9, 2023

josh-m-sharpe commented Feb 9, 2023

maximecb commented Feb 10, 2023

maximecb commented Feb 27, 2023 • edited Loading

XrXr commented Feb 27, 2023 • edited Loading

josh-m-sharpe commented Feb 28, 2023 • edited Loading

XrXr commented Feb 28, 2023

XrXr commented Jan 19, 2023 •

edited

Loading

johnnyshields commented Jan 29, 2023 •

edited

Loading

maximecb commented Feb 27, 2023 •

edited

Loading

XrXr commented Feb 27, 2023 •

edited

Loading

josh-m-sharpe commented Feb 28, 2023 •

edited

Loading