-
Notifications
You must be signed in to change notification settings - Fork 419
Description
* Operating system: mac
* Ruby implementation: Ruby 3.1.0
* `concurrent-ruby` version: 1.1.9
* `concurrent-ruby-ext` installed: no
* `concurrent-ruby-edge` used: no
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. The yield
however causes a jump all the way out of the begin
in SafeTaskExecutor#execute
. If I put an ensure
on that begin, it runs. Nothing after @task.call
in the block runs. No exception can be caught.
I then get this:
project/gems/ruby/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
reason.exception(*args)
^^^^^^^^^^ (NoMethodError)
from project/gems/ruby/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:87:in `raise'
from project/gems/ruby/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 project/gems/ruby/3.1.0/gems/concurrent-ruby-1.1.9/lib/concurrent-ruby/concurrent/concern/obligation.rb:87:in `wait!'
from project/gems/ruby/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:141:in `each'
from project/gems/ruby/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:141:in `find'
from project/gems/ruby/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:153:in `each'
from project/gems/ruby/3.1.0/gems/sprockets-4.0.2/lib/sprockets/manifest.rb:153:in `find_sources'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_loaders/asset_pipeline_loader.rb:13:in `each'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_loaders/asset_pipeline_loader.rb:13:in `first'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_loaders/asset_pipeline_loader.rb:13:in `load'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:48:in `block in load_css'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:46:in `each'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:46:in `load_css'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:20:in `css_for_url'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:13:in `block in css_for_doc'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:13:in `map'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/css_helper.rb:13:in `css_for_doc'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/customized_premailer.rb:14:in `initialize'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:93:in `new'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:93:in `premailer'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:74:in `generate_html_part'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:52:in `generate_html_part_replacement'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:24:in `perform'
from project/gems/ruby/3.1.0/gems/premailer-rails-1.11.1/lib/premailer/rails/hook.rb:8:in `perform'
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!
Activity
agrberg commentedon Jan 8, 2022
Problem
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 thatblock_given?
inSprockets::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. Callingfirst
on that object still causes the same error seen here even though the object responds to the method.Workaround
Changing
.first
to.to_a.first
on this line inpremailer-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.aaronjensen commentedon Jan 9, 2022
@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.
aaronjensen commentedon Jan 9, 2022
Here is a repro:
https://github.com/aaronjensen/concurrent-repro
Install gems, then run
bin/rails c
and do:agrberg commentedon Jan 9, 2022
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
inPromise#realize
is intended to return a triple. However in our case at least bothsuccess
andreason
arenil
. This is why there is no methodexception
on thereason
object.In cases where this is successful,
success
istrue
,value
is set, andreason
isnil
. My hunch is thatsuccess
is intended to befalse
when a properreason
exists vs. encountering this case wheresuccess
is the only other falsey value,nil
. I'll report back if I have any more luck!aaronjensen commentedon Jan 10, 2022
@agrberg Right, because for some reason the
begin
block atconcurrent-ruby/lib/concurrent-ruby/concurrent/executor/safe_task_executor.rb
Lines 23 to 29 in 52c08fc
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
.agrberg commentedon Jan 10, 2022
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 ofPremailer::Rails::CSSHelper::FileNotFound
specifically: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.
aaronjensen commentedon Jan 10, 2022
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.
aaronjensen commentedon Jan 10, 2022
I narrowed the repro down to just concurrent-ruby: https://github.com/aaronjensen/concurrent-repro
aaronjensen commentedon Jan 10, 2022
This was introduced by this commit:
cc @tenderlove
aaronjensen commentedon Jan 13, 2022
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.SafeTaskExecutor respects local jump errors
aaronjensen commentedon Jan 13, 2022
#932 should fix it
chrisseaton commentedon Jan 13, 2022
Thanks will have to find some time to get my head around the issue and the PR.
agrberg commentedon Jan 13, 2022
Awesome job debugging this dude! 🙇
14 remaining items
aaronjensen commentedon Mar 7, 2022
PR was merged, thanks @chrisseaton
eregon commentedon Mar 16, 2022
The test added in #932 is causing various problems, for instance it doesn't work on JRuby (#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.
eregon commentedon Mar 16, 2022
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):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).aaronjensen commentedon Mar 16, 2022
@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).
eregon commentedon Mar 16, 2022
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.
eregon commentedon Mar 18, 2022
So, using the minimal example from mame in https://bugs.ruby-lang.org/issues/18474:
Here are the results:
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:and the break, instead of breaking out of that
each {}
call, it breaks out of the1.times do
call (or thesynchronize 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 notbreak
, because first is implemented literally as: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.
eregon commentedon Mar 18, 2022
If I make a reproducer closer to the actual code, I see that CRuby is still broken :/
Results:
CRuby (3.0 and 3.1) print
:should_not_reach_here!
, which is a semantic bug, if we get to the end ofexecute
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
.eregon commentedon Mar 18, 2022
My conclusion is:
to_enum
and yielding to it from another thread can never work, that is broken code.Only run the CRuby bug-specific test on CRuby
mshappe commentedon Mar 18, 2022
@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!
aaronjensen commentedon Mar 18, 2022
Thanks @eregon, that sounds great.