Fix template compilation concurrency issue #6401
Conversation
|
The idea behind the original implementation is that templates would only be compiled in development mode (which is always single-threaded) or ahead of time in threadsafe mode. What actual scenario triggers this problem? |
|
@wycats - I was observing empirically with a clean generated rails app (with threadsafe mode enabled) that the templates aren't compiled ahead of time. They're compiled on demand, and if there are multiple requests for the same url when it is first requested then you can get two threads going through the compile process at the same time. I verified this by instrumenting the code. And verified that the fix, serializes the access and makes sure a particular template gets compiled precisely once. In practice I've had this happen several times on a production app, where a particular url is permanently damaged, and unable to be used until the next server restart. I wasn't aware of any functionality that allowed templates to be compiled ahead of time - is there such a thing? If so that would be a satisfactory solution. |
| # so compilation and any instance variable modification must | ||
| # be synchronized | ||
| @compile_mutex.synchronize { | ||
| return if @compiled |
There was a problem hiding this comment.
Nevermind. It is necessary.
There was a problem hiding this comment.
That said, can you please add some extra comments above this line? I think almost everyone first reaction is that the duplicated line is useless. :)
|
@wycats this seems correct to me. we don't do ahead of time compilation. |
This change avoids obtaining a lock for templates that are already compiled by using a thread local copy of any templates already used by the thread. There are a few issues with this however: - It breaks ActionView::Template::Resolver#clear_cache, as there's no good way of clearing all the Thread local copies. Within Rails it's only used in one test (which could be implemented another way). So the question is possibly if this is intented to be part of the public API and if it can be removed. - If there's any scenario in which Resolvers can be re-created there is a potential memory leak as there's no attempt to clear up the thread local storage - I don't have any performance data to justify the need to avoid locks
This addresses issue #6400.
I made the change against 3.2.3-rc2, but it also applies almost cleanly against master (just a few whitespace issues) and cleanly against 3.1-stable.