Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store global objects into thread-local variables #711

Merged
merged 2 commits into from
Aug 11, 2024

Conversation

ildarkayumov
Copy link
Contributor

Context

Bullet stores global objects into fiber-local variables before each request, uses these global objects to collect all N+1 queries and later shows all collected problems based on these global objects result. This approach works well until someone don't try to run his code inside Fiber like this

class PostsController < ApplicationController
  def index
    fiber = ::Fiber.new do
      Post.find_each do |post|
        post.comments.each do |comment|
          comment.name
        end
      end
    end

    fiber.resume
  end
end

In that case bullet warning is not shown because all global objects are stored as fiber-local variables for current Fiber and not passed to all new Fibers

Decision

The easiest way is to store bullet objects into thread-local variables instead: https://ruby-doc.org/core-2.6/Thread.html#class-Thread-label-Fiber-local+vs.+Thread-local

Changes

  1. move Bullet objects to thread-local variables
  2. combine variables into single thread local variable

Notes

this problem can easily occur even when rails app does not use fibers explicitly but instead uses some 3rd party libraries like dry-rb/dry-effects#82

@ildarkayumov ildarkayumov changed the title fix: store global objects into thread-local variables Store global objects into thread-local variables Aug 10, 2024
lib/bullet.rb Outdated
def start_request
Thread.current[:bullet_start] = true
Thread.current[:bullet_notification_collector] = Bullet::NotificationCollector.new
thread_local_data[:bullet_start] = true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ildarkayumov I know it's easier to use thread_local_data to replace Thread.current, but I don't want to define the data structure { bullet: { bullet_start: true } }, would you mind using the following code instead

Thread.current.thread_variable_set(:bullet_start, true)

@ildarkayumov ildarkayumov requested a review from flyerhzm August 11, 2024 08:26
@flyerhzm flyerhzm merged commit dddf7ab into flyerhzm:main Aug 11, 2024
9 checks passed
@flyerhzm
Copy link
Owner

@ildarkayumov thank you for fixing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants