rails: Thread safety problem in template loading: "can't add a new key into hash during iteration"

Steps to reproduce

This is a thread safety, as such it cannot be reliably reproduced.

Expected behavior

Template loading and compilation is performed lazily, even when config.eager_load = true. Since this needs to work in production where multiple threads might try to load a template at the same time, it needs to be thread safe.

Actual behavior

Template loading is not thread safe, occasionally causing the following error:

#<ActionView::Template::Error: can't add a new key into hash during iteration>

See full trace here: https://gist.github.com/jnicklas/21b570f19c50d958f5024566dfd052f0

System configuration

Rails version: 5.0.0.beta3 (rev db9bc8097399aab9c86)

Ruby version: ruby 2.3.0p0 (2015-12-25 revision 53290) [x86_64-darwin15]

About this issue

  • Original URL
  • State: closed
  • Created 8 years ago
  • Comments: 43 (33 by maintainers)

Commits related to this issue

Most upvoted comments

This issue still occurs randomly from time to time, I don’t know why but everything work fine until it happens without any reason ! it’s rare but it happens

@thedarkone & @rafaelfranca you’re absolutely right, it’s the exact same error message but a different backtrace. It’s from Sprockets, and will continue the dialog there. Need to pay a little more attention to detail before sending someone off into a code hole

@arktisklada if you are talking about the sprockets issue, it is not related to this one.

I’ve created a ticket over at concurrent-ruby ruby-concurrency/concurrent-ruby#528.

@jeremy, if you have the time, can you take a look at what I have there?

Also of note: it seems that this is a regression. This issue did not occur with earlier versions of Rails, it might be due to the switch from ThreadSafe::Cache to Concurrent::Map, but the project that I’m seeing this error in tracks Rails master and it is unclear when exactly this issue started happening (yay non-determinism).

Tracked this down to a separate issue with hash modification during iteration in Jbuilder. All good, @thedarkone.

@rafaelfranca thank you ❤️! I just spent an hour digging through MRI’s hash.c (didn’t find anything new).

@arktisklada rails/sprockets#242 has nothing to do with this issue or concurrent-ruby. As per backtrace, are you seeing errors from sprockets or Rails?

Bumped to c-r 1.0.2 @ 06dc3fba4623a77e3cc7fa04f97723462d47bf1e

c-r 1.0.2 has been released. It has the aforementioned fix.

It seems that the fix pushed to concurrent-ruby did resolve this issue for us.

@thedarkone Thank you very much for jumping on this so quickly! Your insight is always extremely valuable. I’ll continue the conversation on the c-r thread.

This shouldn’t be happening (obviously 😢).

@matthewd you are correct in assuming the iteration must be happening on a diff thread, I can’t image it to be an in issue of nested @data[key][name][prefix][partial][locals] look ups or anything like that.

On the other hand, the iteration from the other thread should also never cause this! Since the reported issue is happening on MRI, where C::Map is just a glorified thin wrapper over native Hash, it is quite obvious that a thread iterating over a hash (e.g. Hash#each) will trigger a can't add a new key into hash during iteration error on all other threads attempting to insert new keys. However concurrent-ruby is aware of that: on MRI all iterative methods of C::Map pre-dup the underlying Hash when iterating (this is obviously inefficient, especially for large maps, but we more than make up for this by fully relying on MRI’s C-based Hash).

I also just torture tested iteration with insertion on MRI and couldn’t reproduce 😿.


This issue did not occur with earlier versions of Rails, it might be due to the switch from ThreadSafe::Cache to Concurrent::Map.

Concurrent::Map is a copy-pasted and renamed ThreadSafe::Cache. Additionally, there were almost no changes to the code in the last 3-4 years, so this shouldn’t be a version thing either.

@jnicklas can you make sure you’re not using some code that perhaps pries open Concurrent::Map accessing its underlying @backend i-var? You could maybe test that by manually changing concurrent-ruby/lib/concurrent/collection/map/non_concurrent_map_backend.rb and replacing @backend with @__backend__.

PS: Rails doesn’t iterate the said C::Maps, does it?