Closed aaronjensen closed 2 years ago
I'm having the same error via premailer-rails
as well and have been looking into it a bit myself this morning. My difference is that block_given?
in Sprockets::Manifest#find_sources
is false so I have an instance of Enumerable in https://github.com/fphilipe/premailer-rails/blob/v1.11.1/lib/premailer/rails/css_loaders/asset_pipeline_loader.rb#L11. Calling first
on that object still causes the same error seen here even though the object responds to the method.
/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:128:in `exception': undefined method `exception' for nil:NilClass (NoMethodError)
reason.exception(*args)
^^^^^^^^^^
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:87:in `raise'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:87:in `block in wait!'
from <internal:kernel>:90:in `tap'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:87:in `wait!'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:130:in `each'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:130:in `find'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:142:in `each'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:142:in `find_sources'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_loaders/asset_pipeline_loader.rb:12:in `each'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_loaders/asset_pipeline_loader.rb:12:in `first'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_loaders/asset_pipeline_loader.rb:12:in `load'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:47:in `block in load_css'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:46:in `each'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:46:in `load_css'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:20:in `css_for_url'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:13:in `block in css_for_doc'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:13:in `map'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:13:in `css_for_doc'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/customized_premailer.rb:14:in `initialize'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:93:in `new'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:93:in `premailer'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:84:in `generate_text_part'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:62:in `generate_alternative_part'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:50:in `generate_html_part_replacement'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:24:in `perform'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:8:in `perform'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/mail-2.7.1/lib/mail/mail.rb:235:in `block in inform_interceptors'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/mail-2.7.1/lib/mail/mail.rb:234:in `each'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/mail-2.7.1/lib/mail/mail.rb:234:in `inform_interceptors'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/mail-2.7.1/lib/mail/message.rb:248:in `inform_interceptors'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/mail-2.7.1/lib/mail/message.rb:258:in `deliver'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/actionmailer-7.0.1/lib/action_mailer/message_delivery.rb:119:in `block in deliver_now'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/actionmailer-7.0.1/lib/action_mailer/rescuable.rb:17:in `handle_exceptions'
from /.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/actionmailer-7.0.1/lib/action_mailer/message_delivery.rb:118:in `deliver_now'
Changing .first
to .to_a.first
on this line in premailer-rails
allows me to work around this issue. I haven't deployed this so I can only confirm it works in development and with my tests.
@agrberg I have the exact same circumstances. The same fixes it for me. Though it is called without a block initially, I believe that this line ends up recalling the same method with a block.
This seems like a Ruby bug to me or some breaking change I don't know about.
Here is a repro:
https://github.com/aaronjensen/concurrent-repro
Install gems, then run bin/rails c
and do:
::Rails.application.assets_manifest.find_sources("application.css").first
I'm still tracking this down in a few spare moments. Via pry-byebug
I was able to narrow the cause a bit further. SafeTaskExecutor
in Promise#realize
is intended to return a triple. However in our case at least both success
and reason
are nil
. This is why there is no method exception
on the reason
object.
In cases where this is successful, success
is true
, value
is set, and reason
is nil
. My hunch is that success
is intended to be false
when a proper reason
exists vs. encountering this case where success
is the only other falsey value, nil
. I'll report back if I have any more luck!
@agrberg Right, because for some reason the begin
block at https://github.com/ruby-concurrency/concurrent-ruby/blob/52c08fca13cc3811673ea2f6fdb244a0e42e0ebe/lib/concurrent-ruby/concurrent/executor/safe_task_executor.rb#L23-L29 exits without recording an exception or getting to line 24.
The furthest I see things going is https://github.com/rails/sprockets/blob/v4.0.2/lib/sprockets/manifest.rb#L143 and then next thing I know it's returning from SafeTaskExecutor#execute
.
I found the same thing! I disabled the source of the error by changing this line to reason&.exception(*args)
and now get a clearer error of Premailer::Rails::CSSHelper::FileNotFound
specifically:
/.asdf/installs/ruby/3.1.0/lib/ruby/gems/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:51:in `load_css': File with URL "/assets/email.debug-#{long_hash}.css" could not be loaded by any strategy. (Premailer::Rails::CSSHelper::FileNotFound)
My hunch is that there is a race condition between this file being requested/accessed as when I "slow" the process down with my workaround, the information is found and everything works.
I believe that error only happens because of the thing I described. It is not the actual error as far as I can tell, rather the natural consequence of the asset loader returning nil rather than an asset.
I narrowed the repro down to just concurrent-ruby: https://github.com/aaronjensen/concurrent-repro
require "concurrent-ruby"
def sub
yield
end
def run
return to_enum(__method__) unless block_given?
executor = :fast
# This works
# executor = :immediate
promises = [
Concurrent::Promise.execute(executor: executor) do
# This version fails on Ruby 3.0.3 as well
# yield "some-value"
# This version only fails in Ruby 3.1.0
sub do |value|
yield "some-value"
end
end
]
promises.each(&:wait!)
nil
end
# This does not work
run.first
# This works
# run.to_a.first
puts "It worked"
This was introduced by this commit:
938e027cdf019ff2cb6ee8a7229e6d9a4d8fc953 is the first bad commit
commit 938e027cdf019ff2cb6ee8a7229e6d9a4d8fc953
Author: Aaron Patterson <tenderlove@ruby-lang.org>
Date: Tue Jan 26 15:49:21 2021 -0800
Eliminate useless catch tables and nops from lambdas
cc @tenderlove
It's seeming like this is a bug in concurrent-ruby, which does not account for the local jump that happens when an enumerator breaks when first
is used. I'll look into seeing how I might fix it, but it would be great for one of the maintainers to chime in if they are available.
Thanks will have to find some time to get my head around the issue and the PR.
Awesome job debugging this dude! 🙇
This weekend I upgraded my tiny app to Rails 7 and transitioned from Webpacker to importmaps. I exercised the latter by precompiling my assets locally and stumbled onto another work around when attempting the Ruby 3.1 upgrade. The specific case w/ premailer-rails
that @aaronjensen and I have can be avoided in test/development when assets exist on disk! I can also confirm the natural followup question that this isn't a problem in production as assets are statically compiled (I'm using Heroku so YMMV but I'm also not aware of any benefits/strategies where assets aren't precompiled in prod).
The underlying bug still remains where the assets are considered missing when they're intended to be generated dynamically.
@chrisseaton have you had a chance to look at this? This is the only thing blocking us from upgrading to 3.1.0 and I'd rather not have to patch premailer-rails, though if you are thinking of not accepting this, please let me know and I can open a PR there to work around this. Thank you!
@aaronjensen, did you find a solution to make this work while we wait for the PR to be reviewed?
@thomasvanholder because it's ruby, you can redefine SafeTaskExecutor#execute
as I defined it in the PR. You can also monkey-patch what's described in the workaround here: https://github.com/ruby-concurrency/concurrent-ruby/issues/931#issuecomment-1008149312
We have been holding on monkey-patching hoping this gets merged, but we may need to just do it if we want to upgrade Ruby (or submit a workaround to premailer-rails).
@eregon it looks like Chris may be busy, would you be able to take a look at this by chance?
Sorry I will manage to take a look today.
No problem, and thank you.
Hi @chrisseaton and @eregon sorry to prod, but would it be possible to take a look at this?
It would be really good if this could either move forward, or we could find out what else is being done to unblock Ruby 3.1.0 upgrades...
Very sorry! I'll definitely take a look in the next 24 hours.
I can reproduce. Sorry I know that's just a start but evidence I'm on it.
Great, thanks @chrisseaton. Not sure if you saw my PR or not, but it fixes it w/ minimal changes and has a test: https://github.com/ruby-concurrency/concurrent-ruby/pull/932
While there may be an underlying issue in concurrent-ruby
that still needs to be addressed, note that sprockets
v4.0.3 has just been released, which contains this fix.
That’s great news, thank you @tomstuart
PR was merged, thanks @chrisseaton
The test added in https://github.com/ruby-concurrency/concurrent-ruby/pull/932 is causing various problems, for instance it doesn't work on JRuby (https://github.com/ruby-concurrency/concurrent-ruby/pull/932), it doesn't work on TruffleRuby (https://github.com/ruby-concurrency/concurrent-ruby/runs/5554594154?check_suite_focus=true), and a fairly small variation of that test segfaults on CRuby 3.0 (https://bugs.ruby-lang.org/issues/18637) or behaves incorrectly (https://bugs.ruby-lang.org/issues/18474).
That test seems too crazy, it's creating an Enumerator with to_enum
and yielding to it from another thread. That seems to not make sense and inherently always going to cause problems, so I think we need to fix whatever code would try to do that.
I'm inclined to remove the test (cost is too high compared to value, and it's extremely difficult to understand what the test is trying to do), unless we find a way to rewrite so it doesn't involve a yield from another Thread.
Sorry, I missed you reported this issue to CRuby, that gives more details: https://bugs.ruby-lang.org/issues/18474
So in CRuby 3.1 and in TruffleRuby, this code gives a LocalJumpError
(different messages):
puts RUBY_DESCRIPTION
def execute
Thread.new do
yield 42
end.join
end
p first: to_enum(:execute).first
ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [x86_64-linux]
#<Thread:0x00007f9ae70a4590 repro.rb:17 run> terminated with exception (report_on_exception is true):
unexpected break (LocalJumpError)
repro.rb:19:in `join': unexpected break (LocalJumpError)
from repro.rb:19:in `execute'
from repro.rb:22:in `each'
from repro.rb:22:in `first'
from repro.rb:22:in `<main>'
truffleruby 22.1.0-dev-fc85fe78, like ruby 3.0.2, GraalVM CE Native [x86_64-linux]
#<Thread:0xc8 repro.rb:17 run> terminated with exception:
Traceback (most recent call last):
from <internal:core> core/truffle/thread_operations.rb:164:in `report_exception'
from <internal:core> core/exception.rb:105:in `full_message'
<internal:core> core/thread.rb:135:in `initialize': unexpected return (LocalJumpError)
<internal:core> core/thread.rb:135:in `initialize': unexpected return (LocalJumpError)
from <internal:core> core/exception.rb:105:in `full_message'
from <internal:core> core/truffle/thread_operations.rb:164:in `report_exception'
I think what we'd need is to tweak the test to just raise LocalJumpError
explicitly for clarity and reproducibility (e.g., otherwise it works incorrectly on CRuby 3.0).
@eregon If you could instead limit the test to running only on the affected environment (CRuby >= 3.1) then it could maintain the shape of the actual problem. I don't know if just raising LocalJumpError actually does what you would think (I believe I tried that, but I could be mistaken).
Good point, I'll try to repro the original problem by locally reverting the fix on CRuby 3.1, and see if I can simplify the test.
So, using the minimal example from mame in https://bugs.ruby-lang.org/issues/18474:
def foo
Thread.new do
1.times do
yield 42
end
p "should_not_reach_here"
end.join
end
p to_enum(:foo).first
Here are the results:
$ ruby -v repro.rb
ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-linux]
"should_not_reach_here"
42
ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [x86_64-linux]
#<Thread:0x00007f98548c45f8 repro.rb:2 run> terminated with exception (report_on_exception is true):
unexpected break (LocalJumpError)
repro.rb:7:in `join': unexpected break (LocalJumpError)
from repro.rb:7:in `foo'
from repro.rb:10:in `each'
from repro.rb:10:in `first'
from repro.rb:10:in `<main>'
truffleruby 22.1.0-dev-a5b55044, like ruby 3.0.2, GraalVM CE Native [x86_64-linux]
#<Thread:0xf8 repro.rb:2 run> terminated with exception:
Traceback (most recent call last):
from <internal:core> core/truffle/thread_operations.rb:164:in `report_exception'
from <internal:core> core/exception.rb:91:in `full_message'
from <internal:core> core/truffle/exception_operations.rb:138:in `full_message'
<internal:core> core/thread.rb:135:in `initialize': unexpected return (LocalJumpError)
<internal:core> core/thread.rb:135:in `initialize': unexpected return (LocalJumpError)
from <internal:core> core/truffle/exception_operations.rb:138:in `full_message'
from <internal:core> core/exception.rb:91:in `full_message'
from <internal:core> core/truffle/thread_operations.rb:164:in `report_exception'
jruby 9.3.2.0 (2.6.8) 2021-12-01 0b8223f905 OpenJDK 64-Bit Server VM 11.0.14.1+1 on 11.0.14.1+1 +jit [linux-x86_64]
warning: thread "Ruby-0-Thread-1: repro.rb:1" terminated with exception (report_on_exception is true):
ThreadError: Enumerable#first cannot be parallelized
foo at repro.rb:4
times at org/jruby/RubyFixnum.java:302
foo at repro.rb:3
ThreadError: Enumerable#first cannot be parallelized
foo at repro.rb:4
times at org/jruby/RubyFixnum.java:302
foo at repro.rb:3
CRuby 3.0 behavior is a clear bug, it prints "should_not_reach_here"
which is semantically completely broken.
What most likely happens is on CRuby Enumerable#first uses break
internally, like if we wrote it like:
def first
each { |e| break e } # and somehow return nil if no element not sure how CRuby does that but irrelevant here
end
and the break, instead of breaking out of that each {}
call, it breaks out of the 1.times do
call (or the synchronize do
call in concurrent-ruby).
CRuby 3.1 doesn't have this bug, instead it's a LocalJumpError which means it can't find where to break, and indeed it would need to break/jump to another thread stack, which is of course impossible.
TruffleRuby also raises a LocalJumpError but it's about return
and not break
, because first is implemented literally as:
def first(n=undefined)
return __take__(n) unless Primitive.undefined?(n)
each do
o = Primitive.single_block_arg
return o
end
nil
end
JRuby uses a separate exception, a ThreadError, which is arguably more helpful, so that behavior sounds fine too.
So it's a bug of CRuby < 3.1. I still need to check why the test doesn't pass on other impls/versions though.
If I make a reproducer closer to the actual code, I see that CRuby is still broken :/
def synchronize
yield
end
def execute(task)
success = true
value = reason = nil
end_sync = false
synchronize do
begin
p :before
value = task.call
p :never_reached
success = true
rescue StandardError => ex
p [:rescue, ex]
reason = ex
success = false
end
end_sync = true
p :end_sync
end
p :should_not_reach_here! unless end_sync
[success, value, reason]
end
def foo
Thread.new do
result = execute(-> { yield 42 })
p [:result, result]
end.join
end
p [:first, to_enum(:foo).first]
Results:
ruby 3.0.3p157 (2021-11-24 revision 3fb7d2cadc) [x86_64-linux]
:before
:should_not_reach_here!
[:result, [true, nil, nil]]
[:first, 42]
ruby 3.1.1p18 (2022-02-18 revision 53f5fc4236) [x86_64-linux]
:before
:should_not_reach_here!
[:result, [true, nil, nil]]
[:first, 42]
:before
#<Thread:0xc8 repro2.rb:31 run> terminated with exception:
Traceback (most recent call last):
from <internal:core> core/truffle/thread_operations.rb:164:in `report_exception'
from <internal:core> core/exception.rb:91:in `full_message'
from <internal:core> core/truffle/exception_operations.rb:138:in `full_message'
<internal:core> core/thread.rb:135:in `initialize': unexpected return (LocalJumpError)
<internal:core> core/thread.rb:135:in `initialize': unexpected return (LocalJumpError)
from <internal:core> core/truffle/exception_operations.rb:138:in `full_message'
from <internal:core> core/exception.rb:91:in `full_message'
from <internal:core> core/truffle/thread_operations.rb:164:in `report_exception'
jruby 9.3.2.0 (2.6.8) 2021-12-01 0b8223f905 OpenJDK 64-Bit Server VM 11.0.14.1+1 on 11.0.14.1+1 +jit [linux-x86_64]
:before
[:rescue, #<ThreadError: Enumerable#first cannot be parallelized>]
:end_sync
[:result, [false, nil, #<ThreadError: Enumerable#first cannot be parallelized>]]
[:first, nil]
CRuby (3.0 and 3.1) print :should_not_reach_here!
, which is a semantic bug, if we get to the end of execute
we should have gotten to the end of the block given to synchronize.
TruffleRuby prints the LocalJumpError, and then apparently it gets transferred to the main thread, I'm not sure how, but at least it shows the bug in the code.
JRuby does what I expect here, it raises a LocalJumpError early and that's caught by the rescue
.
My conclusion is:
to_enum
and yielding to it from another thread can never work, that is broken code.@eregon Thank you for investigating this and for your very complete explanation of what was going on. I suspect CRuby's behaviour comes down to the reality that CRuby's thread handling is, and always has been, not-really-thread handling!
Thanks @eregon, that sounds great.
After upgrading to Ruby 3.1.0 I'm getting a very strange, very hard (for me) to debug error with premailer+sprockets that ultimately manifests with concurrent failing to provide a reason for a promise rejection.
The error is strange enough that it seems like it could be a ruby bug.
When stepping through with byebug I get here:
https://github.com/rails/sprockets/blob/v4.0.2/lib/sprockets/manifest.rb#L143
Then, drilling into
asset.source
, it returns the source fine. Theyield
however causes a jump all the way out of thebegin
inSafeTaskExecutor#execute
. If I put anensure
on that begin, it runs. Nothing after@task.call
in the block runs. No exception can be caught.I then get this:
I may be able to narrow down a repro if that would be useful, but I wanted to file this now in case there are already any known issues or work arounds I'm not aware of.
Thanks!