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

render stream: true always use default locale in layout #30494

Closed
mfo opened this issue Sep 1, 2017 · 1 comment
Closed

render stream: true always use default locale in layout #30494

mfo opened this issue Sep 1, 2017 · 1 comment

Comments

@mfo
Copy link
Contributor

mfo commented Sep 1, 2017

Steps to reproduce

I have a waiting PR on this issue, but It's stalled. So I'm here to check if it's something you plan to support.

Here is some background :

  1. render stream:true is implemented via ActionView::StreamingTemplateRenderer. The layouts is rendered within a Fiber:
fiber = Fiber.new do
  if layout
    layout.render(view, locals, output, &yielder)
  else
    # If you don't have a layout, just render the thing
    # and concatenate the final result. This is the same
    # as a layout with just <%= yield %>
    output.safe_concat view._layout_for
  end
end

But Fibers can't access Thread.current[]. ex:

irb
> Thread.current[:foo]= :bar
> Fiber.new { puts Thread.current[:foo] }.resume # puts nil...
  1. Some parts of the stack rely on Thread.current[]... ex: the I18n which stores the current I18n config in Thread.current[:i18n_config].
  2. the result is I18n(.locale, I18n.t, t(), ...) does not works within the layout (and all the call which rely on a Thread.current[]).

Expected behavior

You tell me.

A few ideas:

  1. Instead of using a classic Fiber, use a custom one that copy the caller's I18n_config in its scope:
class FiberWithI18n < Fiber
      def self.new_with(caller_i18n_config = I18n.config, &block)
        new do
          I18n.config = caller_i18n_config
          block.call
        end
      end
    end
  1. Instead of using a classic Fiber, use a custom one that copy the caller's Thread.current[] in its scope:
class FiberWithLocalsFromCallerThread < Fiber
  def self.new_with(caller_thread = Thread.current, &block)
    caller_thread_locals = extract_thread_locals(caller_thread)
    new do
      reapply_locals(Thread.current, caller_thread_locals)
      block.call
    end
  end

  def self.extract_thread_locals(thread)
    thread.keys.inject({}) do |memo, thread_local_varname|
      memo[thread_local_varname] = thread[thread_local_varname]
      memo
    end
  end

  def self.reapply_locals(thread, locals)
    locals.each_pair do|key, value|
      thread[key] = value
    end
  end
end
  1. Drop the Fiber (which is the root cause of this bug) and use a threaded model.

Actual behavior

Within the layout we loose all track of Thread.current[]

System configuration

Rails version: I tested with 5.1.3
Ruby version: I tested with 2.4.1p111
But I'm not sure it's relevant, Fiber does not support access to Thread.current, so same behaviour since the introduction of the Fiber.

@mfo
Copy link
Contributor Author

mfo commented Sep 14, 2017

Ok maybe I'm simply wrong about the expected feature of the render stream :true. But I want to flush my response as earlier as possible (via a Transfer-Encoding: chunked).

I tried to reproduce this ?bug? again... with a brand new rails app. Its fails : https://github.com/mfo/rails_30361/tree/master/app.

So I should be wrong, anyone to give an hint ? What am I missing (I'll glady hear your worse feedback).

@mfo mfo closed this as completed Nov 25, 2017
@mfo mfo reopened this Dec 1, 2017
@mfo mfo closed this as completed Dec 1, 2017
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

No branches or pull requests

1 participant