Connection pool and context switching

[udovenko]

I've looked at ConnectionPool and ReactOnRails codebase, but still did not find an answer. ReactOnRails allows to separately initialize Redux store and React components with redux_store and react_component helpers on server side. My question is: is there possibility of Ruby context switching between these helpers calls which potentially can lead to rehydration of Redux store by another thread before react component is rendered?
For example, in my template I have someting like this:

<%= redux_store('Store', {....some props}) %>
...
<%= react_component('App', id: 'react-root')  %>
...
<%= server_render_js('DocumentMeta.renderAsHTML();')%>

So template logic checks out js_context from the pool, hydrates store there, then renders React component assuming that we're in the same js_context with properly hydrated store, then retrieves meta tags rendered by DocumentMeta package during component rendering.

This approach works fine without concurrency. But is the following scenario possible in multithreading environment:

1. Thread 1 hydrates the store;
2. Context switches to Thread 2, it checks out the same js_context and hydrates store with different data;
3. Context switches back to Thread 1 and it renders react component with wrong data?

Sorry if this is a stupid question and the answer obvious...

[justin808]

@udovenko Hydration of the redux store is per request, and is not long lived.

[udovenko]

@justin808 Sorry but it seems you did not understand my question. I'm talking about pool behaviour within the single request. Ruby can switch context at any moment. And I can't see any code that prevents js_context form checking in back to the pool during single request. So another parallel request in multithreading environment can chek the same js_context out and change it before request one is finshed.

[udovenko]

It is pretty sad that issue was closed before I even had a chance to explain what I mean...

[justin808]

@udovenko Very easy to reopen the issue. We use the same connection pool algorithm of react-rails. Thread will get control and return the object when done. If there was a real issue, somebody would probably have seen it by now.

Do you agree? In other words, if we didn't use a connection pool, we would have issues.

[udovenko]

@justin808 Thank you for response! I did not use react-rails and I'm not sure it this gem offers an ability to hydrate store in a separate #with call to ConnectionPool.
Please correct me if I'm wrong: every time we call helper methods like #redux_store or #react_component, #with method is called for ConnectionPool and this method receives a block with code that manipulates yelded js_context. Once code in a block is executed, updated js_context will return to the pool. So js_context can be theoreticaly changed between helper method calls within single request if another thread will check it out.
I know there are no real issues, but If my doubts have a real reason, this is a threadsafety issue and it is really hard to reproduce. It can appear rarely and only under serious load in multithreading environment.

[justin808]

@udovenko can you try to see if you can reproduce the issue? Maybe put in sleeps? Print statements?

[udovenko]

@justin808 Ok, I will try to do it this week. I'll start from pure ConnectionPool with dummy objects since I do not see any special logic added by ReactOnRails gem around it.

[udovenko]

@justin808 Ok, here is a quick and dirty test for ConnectionPool behaviour:

require 'connection_pool'

$connection_pool = ConnectionPool.new(size: 1, timeout: 5) { { dummy_js_context: '' } }

class DummyRequest
  def initialize(id, sleep_time)
    @id = id
    @sleep_time = sleep_time
  end

  # Imagine this method is our request call stack:
  def perform
    redux_store
    sleep(@sleep_time)
    react_component
  end

  private

  def redux_store
    $connection_pool.with do |context_container|
      context_container[:dummy_js_context] = "JS Context changed by request##{@id}"
      p context_container[:dummy_js_context]
    end
  end

  def react_component
    $connection_pool.with do |context_container|
      p "Request##{@id} renderd React component in #{context_container[:dummy_js_context]}"
    end
  end
end

thread_1 = Thread.new do
  request = DummyRequest.new(1, 1)
  request.perform
end.run

thread_2 = Thread.new do
  request = DummyRequest.new(2, 0)
  request.perform
end.run

thread_1.join
thread_2.join

An output from this snippet is:

"JS Context changed by request#1"
"JS Context changed by request#2"
"Request#2 renderd React component in JS Context changed by request#2"
"Request#1 renderd React component in JS Context changed by request#2"

Imagine these simple threads are our parallel requests. First request takes one second to handle logic between helper method calls while second request works without delay. The last print in output shows that React component in the first request will be rendered in wrong context, changed by concurent second request.

I have no idea on how to test it with real Rails app yet, but I do not see any reason for it to behave differently.

[justin808]

@udovenko what do you recommend as the fix?

[udovenko]

@justin808 First of all - create falling tests for this particular case within Rails stack. I have no experience wih testing concurent requests with RSpec, but I think controller specs is the first place to try.

Then, if issue is confirmed by falling tests, all helper methods should be moved inside the same #with block of ConnectionPool. Which is uneasy task. Personally I see 3 options, but in all cases #with call should be removed form .server_render_js_with_console_logging method of ServerRenderingPool (or from .eval_js to be precise):

• If we really want to check out js_context per request, maybe some additional Rack Middleware could be used to wrap the whole request in #with block. But request can contain a lot of additional long operations not reated to Redux store and React components rendering. And concurent requests will have to wait for js_context to be checked in too long.
• Force developers to manually call #with block around ReactOnRails helpers. It could be complicated especially when redux_store helper is called from controller.
• Introduce your own ConnectionPool.

[justin808]

@udovenko I think we should do something so that if a thread picks up a transaction that's in process, due to a call to redux_store before calling redux_component, then the thread should return the connection and keep trying to get a new connection until it gets one that is not in use.

The tricky part is going to be that we'll some sort of rack middleware or controller after hook to clear out this static flag, and if the controller does hit a ruby error, etc., then we need to be sure that we still clear out the flag.

Thoughts?

[udovenko]

@justin808 If I understand you correctly, the procedure you've described is already implemented by ConnectionPool by #with method. Any code that runs inside this block will wait for available js_context until it will be released by other threads or untill timeout is exceed (then an error is rised).
What is the reason for setting a static flag? The problem is that instead of wrapping all helper methods within the single request in same #with block each helper method calls its own #with block from inside and returns js_context to pool right when finished. Please look at https://github.com/mperham/connection_pool/blob/master/lib/connection_pool.rb to see how #with method works.

[robwise]

@udovenko But don't parallel requests get different connection_pool instances (not threads, the entire pool is different) anyway, so there's no way one request could corrupt the connection pool of a different request?

[udovenko]

@robwise As I can see pool object is a class variable: https://github.com/shakacode/react_on_rails/blob/master/lib/react_on_rails/server_rendering_pool/exec.rb So it should be shared across the threads.

[udovenko]

@robwise ReactOnRails team recommended Puma as a server in the boilerplate project. This is a multithreading server. It loads application in a parent process and launches new thread for each new request. Please correct me if i'm wrong. Parent process (Puma worker) stores single class object of ServerRenderingPool. And this class object has variable that points to ConnectionPool instance in memory.

[udovenko]

As I can remember I've even experienced an issue when store state from previous request was affecting next request. At least I thought that was the reason for my bug that time...

[udovenko]

It is easy to check by runing app on Puma with preload_app = true setting and printing object_id of ConnectionPool instance from gem codebase on each request. object_id should be the same. Unfortunatenly I can't check it right now myself...

[robwise]

@udovenko No I think you're right, I thought we were creating a new instance of connection pool per request instead of keeping it in a class var like that. I'm trying to back and read through the source to figure it out.

[robwise]

But I'm still getting lost as to how a new request can affect the store data of a different request? Isn't that held in an instance variable in react_on_rails_helper.rb?

[udovenko]

I mean store data that is in js_context. JS object in Redux store. This object is persistent in its context object until .reset_pool call.

[robwise]

When server rendering, we put each redux_store call's props into a hash here that is an instance variable and therefore unique to , and isolated from, the request: https://github.com/shakacode/react_on_rails/blob/master/app/helpers/react_on_rails_helper.rb#L158

Then each time you call react_component (assuming it's server rendered), we create a thread and do the store initialization and component rendering of that component all in the same thread. https://github.com/shakacode/react_on_rails/blob/master/app/helpers/react_on_rails_helper.rb#L293

In other words, we re-initialize the store data each time for each component.

[justin808]

@udovenko You're 100% right on the issue and I considered this in the original design a long time ago:

https://github.com/shakacode/react_on_rails/blob/master/app/helpers/react_on_rails_helper.rb#L293

The redux store is set at the beginning of each component rendering.

The only the that we should consider is clearing the memory of the redux stores.

  # Returns Array [0]: html, [1]: script to console log
  # NOTE, these are NOT html_safe!
  def server_rendered_react_component_html(
    props, react_component_name, dom_id,
    prerender: required("prerender"),
    trace: required("trace"),
    raise_on_prerender_error: required("raise_on_prerender_error")
  )
    return { "html" => "", "consoleReplayScript" => "" } unless prerender

    # On server `location` option is added (`location = request.fullpath`)
    # React Router needs this to match the current route

    # Make sure that we use up-to-date bundle file used for server rendering, which is defined
    # by config file value for config.server_bundle_js_file
    ReactOnRails::ServerRenderingPool.reset_pool_if_server_bundle_was_modified

    # Since this code is not inserted on a web page, we don't need to escape props
    #
    # However, as JSON (returned from `props_string(props)`) isn't JavaScript,
    # but we want treat it as such, we need to compensate for the difference.
    #
    # \u2028 and \u2029 are valid characters in strings in JSON, but are treated
    # as newline separators in JavaScript. As no newlines are allowed in
    # strings in JavaScript, this causes an exception.
    #
    # We fix this by replacing these unicode characters with their escaped versions.
    # This should be safe, as the only place they can appear is in strings anyway.
    #
    # Read more here: http://timelessrepo.com/json-isnt-a-javascript-subset

    wrapper_js = <<-JS
(function() {
  var railsContext = #{rails_context(server_side: true).to_json};
#{initialize_redux_stores}
  var props = #{props_string(props).gsub("\u2028", '\u2028').gsub("\u2029", '\u2029')};
  return ReactOnRails.serverRenderReactComponent({
    name: '#{react_component_name}',
    domNodeId: '#{dom_id}',
    props: props,
    trace: #{trace},
    railsContext: railsContext
  });
})()
    JS

    result = ReactOnRails::ServerRenderingPool.server_render_js_with_console_logging(wrapper_js)

    if result["hasErrors"] && raise_on_prerender_error
      # We caught this exception on our backtrace handler
      # rubocop:disable Style/RaiseArgs
      raise ReactOnRails::PrerenderError.new(component_name: react_component_name,
                                             # Sanitize as this might be browser logged
                                             props: sanitized_props_string(props),
                                             err: nil,
                                             js_code: wrapper_js,
                                             console_messages: result["consoleReplayScript"])
      # rubocop:enable Style/RaiseArgs
    end
    result
  rescue ExecJS::ProgramError => err
    # This error came from execJs
    # rubocop:disable Style/RaiseArgs
    raise ReactOnRails::PrerenderError.new(component_name: react_component_name,
                                           # Sanitize as this might be browser logged
                                           props: sanitized_props_string(props),
                                           err: err,
                                           js_code: wrapper_js)
    # rubocop:enable Style/RaiseArgs
  end

  def initialize_redux_stores
    return "" unless @registered_stores.present? || @registered_stores_defer_render.present?
    declarations = "var reduxProps, store, storeGenerator;\n"

    all_stores = (@registered_stores || []) + (@registered_stores_defer_render || [])

    result = all_stores.each_with_object(declarations) do |redux_store_data, memo|
      store_name = redux_store_data[:store_name]
      props = props_string(redux_store_data[:props])
      memo << <<-JS
reduxProps = #{props};
storeGenerator = ReactOnRails.getStoreGenerator('#{store_name}');
store = storeGenerator(reduxProps, railsContext);
ReactOnRails.setStore('#{store_name}', store);
      JS
    end
    result
  end

[justin808]

@udovenko given my last response, I'm going to close this issue with your permission. However, I'll open a new issue and suggest that we prepend a call to clear all stores here, just to be sure we don't have any stores from other requests sitting around.

https://github.com/shakacode/react_on_rails/blob/master/app/helpers/react_on_rails_helper.rb#L346

[udovenko]

@robwise Now I see what I was missing. Store rehydrates on each #server_rendered_react_component_html call...

[udovenko]

Yes, this makes the issue resolved... Sorry if I was wasting your time with my doubts.

[justin808]

@udovenko Would you like to address #780? I'd be thrilled to get the help.

[udovenko]

Will try.

[justin808]

@udovenko Please send me your email, and I'll add you to our slack room. BTW, it was some serious work building the separate JS package to interact with the generated JS code. 😃