view_component: view_component + turbo + active_record callbacks can lead to a deadlock

Steps to reproduce

This is a tough one to reproduce as it deals with multithreading and occurs in a private application that I will need to spend a chunk of time to obfuscate / minimize. I can do that if useful, but I figured I’d open the issue to start discussion.

The general setup:

A Rails application
A multithreaded web server
Perform parallel web requests
One web request modifies data inside of an Active Record transaction
- There is an after_commit callback that performs a websocket broadcast
- The broadcast uses view_components to generate HTML
Another web request uses view_components to generate HTML
- It gathers data from Active Record

Expected behavior

No deadlock occurs.

Actual behavior

A deadlock occurs.

Notes / debugging

This stacktrace shows one thread acquires the Active Record connection lock, then attempts to get the view_component compiler lock

.../ruby-3.0.4/gems/rspec-core-3.11.0/exe/rspec:4:in `<top (required)>'
.../project/spec/features/my_work_spec.rb:163:in `block (3 levels) in <top (required)>'
.../ruby-3.0.4/gems/factory_bot-6.2.0/lib/factory_bot/evaluation.rb:18:in `create'
.../ruby-3.0.4/gems/activerecord-6.1.6.1/lib/active_record/transactions.rb:302:in `save!'
.../ruby-3.0.4/gems/activerecord-6.1.6.1/lib/active_record/connection_adapters/abstract/database_statements.rb:320:in `transaction'
.../ruby-3.0.4/gems/activerecord-6.1.6.1/lib/active_record/connection_adapters/abstract/transaction.rb:317:in `within_new_transaction'
.../ruby-3.0.4/gems/activesupport-6.1.6.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
.../ruby-3.0.4/gems/activerecord-6.1.6.1/lib/active_record/connection_adapters/abstract/transaction.rb:155:in `commit_records'
.../ruby-3.0.4/gems/activerecord-6.1.6.1/lib/active_record/transactions.rb:321:in `committed!'
.../ruby-3.0.4/gems/activesupport-6.1.6.1/lib/active_support/callbacks.rb:824:in `_run_commit_callbacks'
.../project/app/models/task.rb:46:in `block in <class:Task>'
.../ruby-3.0.4/gems/turbo-rails-1.1.1/app/models/concerns/turbo/broadcastable.rb:138:in `broadcast_update_to'
.../ruby-3.0.4/gems/actionpack-6.1.6.1/lib/action_controller/renderer.rb:100:in `render'
.../project/app/views/my_work/tasks/_updated_task.html.slim:5:in `_app_views_my_work_tasks__updated_task_html_slim___4242759950573012154_88260'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/base.rb:129:in `render_in'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:126:in `render_template_for'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:95:in `with_lock'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:95:in `synchronize'

This stacktrace shows one thread acquires the view_component compiler lock, then attempts to get the Active Record connection lock

.../ruby-3.0.4/gems/puma-5.6.4/lib/puma/thread_pool.rb:147:in `block in spawn_thread'
.../ruby-3.0.4/gems/actionpack-6.1.6.1/lib/action_controller/metal/rendering.rb:30:in `process_action'
.../project/app/views/my_work/overdue_tasks/index.html.slim:2:in `block in _app_views_my_work_overdue_tasks_index_html_slim___704223861362102887_91000'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/base.rb:129:in `render_in'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:126:in `render_template_for'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:95:in `with_lock'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:95:in `synchronize'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:128:in `block in render_template_for'
.../project/app/components/my_work_task_section_component.html.slim:6:in `call'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/base.rb:129:in `render_in'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:126:in `render_template_for'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:95:in `with_lock'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:95:in `synchronize'
.../ruby-3.0.4/gems/view_component-2.62.0/lib/view_component/compiler.rb:128:in `block in render_template_for'
.../project/app/components/my_work/task_component.html.slim:18:in `call'
.../ruby-3.0.4/gems/activerecord-6.1.6.1/lib/active_record/querying.rb:47:in `find_by_sql'
.../ruby-3.0.4/gems/activerecord-6.1.6.1/lib/active_record/connection_adapters/abstract/query_cache.rb:103:in `select_all'
.../ruby-3.0.4/gems/activerecord-6.1.6.1/lib/active_record/connection_adapters/postgresql_adapter.rb:736:in `prepare_statement'
.../ruby-3.0.4/gems/activesupport-6.1.6.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'

(These abbreviated stacktraces show the general problem; I’d be happy to share the full versions without any removed frames if useful).

I see that the monitor synchronization is optional (via #1177) so I hacked the view_component source to disable the lock even in test mode. I was then able to run our tests 20 times without a deadlock. Without that, our tests deadlock fairly reliably.

System configuration

Rails version: 6.1.4

Puma version: 5.6.4

turbo-rails version: 1.1.1

Ruby version: ruby 3.0.4p208 (2022-04-12 revision 3fa771dded) [arm64-darwin21]

view_component version: 2.62.0

About this issue

Original URL
State: closed
Created 2 years ago
Reactions: 4
Comments: 20 (12 by maintainers)

Commits related to this issue

Fix compiler deadlock Currently there's a deadlock in the compiler due to rendering locking, which can lead to edgecases where other locks prevent the completion of rendering and the inability to com... — committed to ViewComponent/view_component by BlakeWilliams 2 years ago
Fix compiler deadlock (#1448) * Fix compiler deadlock Currently there's a deadlock in the compiler due to rendering locking, which can lead to edgecases where other locks prevent the completion o... — committed to ViewComponent/view_component by BlakeWilliams 2 years ago
Fix compiler deadlock (#1448) * Fix compiler deadlock Currently there's a deadlock in the compiler due to rendering locking, which can lead to edgecases where other locks prevent the completion o... — committed to claudiob/view_component by BlakeWilliams 2 years ago

Most upvoted comments

I just wanted to say thank you. I rather dreaded opening this issue, expecting to be told that I was Doing It Wrong or that ultimately Rails needed to be changed, not VC. Instead, I had the opportunity to have solid technical discussions and my issue was resolved in less than 24 hours. It has been a pleasure!

shepmaster on Aug 3, 2022

Here’s why I think we need a read lock:

Imagine you have a component, MyComponent that is already compiled in a development environment. We’re running Puma with 2 workers, so we effectively have 2 threads that can run concurrently:

Thread 1:

render MyComponent is called, and #compile will return early because it’s already compiled
Jump to thread 2…

Thread 2:

You hit cmd+s on a my_component.html.erb, the compile cache is invalidated
We don’t early exit because the compile cache says we aren’t compiled
The template is compiled, then we undefined the method
Back over Thread 1:

Thread 1:

We got past before_render and render?, and now we’re ready to call the user’s template code! We go to call render_template_for but it doesn’t exist! Boom, NoMethodError
Back to thread 2

Thread 2:

We’ve re-added the render_template_for method but it’s too late, we’ve already caused thread 1 to error.

This is definitely somewhat contrived, but it’s a situation that was relatively common for folks to hit in the CI suites according to this issue.

If we had a read lock that would wait for the compiler in Thread 1, this issue wouldn’t occur since it ensures that the method exists.

BlakeWilliams on Aug 2, 2022

@BlakeWilliams ah ok I’m convinced! Nice explanation 😄

camertron on Aug 2, 2022

Maybe https://www.rubydoc.info/gems/activesupport/Module:silence_redefinition_of_method is relevant here? 🤔

BlakeWilliams on Aug 2, 2022

I’ll test with your PR in a second.

I was able to run 400 tests without a deadlock occurring, so that PR (at ab6fc4e06be41aa89ffd50e9ba93cfc025063b2a) seems like it addresses our problem as well.

shepmaster on Aug 2, 2022

The compiler used to live in the base class, but was extracted to help readability and understandability of the code, since the base class does a lot.

As-is, I think the existing code is roughly equivalent to your examples, but just lives in a separate class since there should be only one compiler per-component as defined in the base class:

https://github.com/github/view_component/blob/ab6fc4e06be41aa89ffd50e9ba93cfc025063b2a/lib/view_component/base.rb#L527-L529

BlakeWilliams on Aug 2, 2022

Since y’all are here, what exactly is the shared resource that is intended to be protected by __vc_compiler_lock?

The render_template_for method, which is defined/undefined on the component class (effectively a singleton resource) https://github.com/github/view_component/blob/ab6fc4e06be41aa89ffd50e9ba93cfc025063b2a/lib/view_component/compiler.rb#L135-L146

I’m writing up something a bit longer, but I think the thread-local compiler approach would still run into issues where one thread undefines the method while another thread is attempting to use it to render.

BlakeWilliams on Aug 2, 2022

@camertron I believe we want to block on read because of the in-between state of a component during #compile where render_template_for (or #call, I don’t remember which) isn’t defined and results in a NoMethodError

BlakeWilliams on Aug 2, 2022

@shepmaster I think that could work too!

I took poked around a bit and ended up reproducing a deadlock with https://github.com/github/view_component/pull/1448 the ReadWriteLock fixed it. I think that may be close to your scenario, but feel free to give that branch a try and see if it resolves your issues in CI to validate it.

BlakeWilliams on Aug 2, 2022