We are currently migrating the Cucumber driver of a big Ruby on Rails 2.3 application in my company. We are migrating from Webrat to Capybara.
One big difference between them is that Capybara run in a thread so in the main thread your application is running, and in another thread Capybara is executing the find, click and all other Capybara methods.
Getting randomly undefined method `collect!’ for nil:NilClass
My amazing colleague Andrea spends almost the day in order to understand what happen and where it happens.
This error is raised from the ActiveRecord::Base.find_by_sql method.
When this method is call, sometimes connection.select_all returns nil and then the undefined method `collect!’ for nil:NilClass error is raised.
The reason why this select_all method returns nil sometimes is due to the ActiveRecord::ConnectionAdapters::QueryCache module which is not written thread safe.
Let say you have a Cucumber step which in the end execute some SELECT on a users table and a second one which executes an UPDATE of one user. The first one runs in the main thread while the second one runs in the capybara thread.
This UPDATE will execute the ActiveRecord::ConnectionAdapters::QueryCache callbacks which will executes the clear_query_cache method which reset the @query_cache variable.
Now look carefully the cache_sql method:
Line 3 @query_cache.has_key? is called and if it returns true then line 5 the query result is fetched from the cache using ` @query_cache[sql]`.
This code is not thread safe because in our case the @query_cache variable is cleared between the call to @query_cache.has_key? and ` @query_cache[sql].
So the method is returning nil and this nil is returned in the end by the connection.select_all` (in the ActiveRecord::Base.find_by_sql method) and the error is raised.
The fix
It’s really easy to fix this unsafe code. We don’t really care if the cache has a key or not. We just want to read the value of the key. If the key is not existing then a nil will be returned so here is the new code:
Basically we:
- removed the check ‘if the key exists’
- read the value from the cache for the key
- in the case the cache returned something we return it otherwise the query is executed and the result is stored in the cache
We created in our project a Rails initializer which monkey patch the ActiveRecord::ConnectionAdapters::QueryCache.cache_sql with the fix.
It’s now working !
What a happiness Andrea and me felt to see the tests passing again ! It was a nice challenge and we succeed. :-)
Hopefully this could help someone.
Update: I have created a Pull Request in order to fix the issue in Rails directly.
