Better double checked locking in load_schema

[casperisfine]

So we've witnessed some weird issues in production. Some serialized attributes ended up not being deserialized from time to time.

After investigating a bit, it end up being the fallout of the following exception:

`connect': Unknown MySQL server host 'XXXXXXXX' (17) (Mysql2::Error::ConnectionError)
    from bundler/gems/rails-9c6bdb54a341/activesupport/lib/active_support/core_ext/benchmark.rb:14:in `block in ms'
    from /usr/lib/ruby-shopify/ruby-shopify-2.6.4/lib/ruby/2.6.0/benchmark.rb:308:in `realtime'
    from bundler/gems/rails-9c6bdb54a341/activesupport/lib/active_support/core_ext/benchmark.rb:14:in `ms'
    from lib/patches/01_mysql_monitoring.rb:15:in `raw_connect'
    [semian]
    from gems/mysql2-0.5.2/lib/mysql2/client.rb:90:in `initialize'
    from gems/activerecord-pedantmysql2-adapter-1.1.3/lib/active_record/connection_adapters/pedant_mysql2_adapter.rb:14:in `new'
    from gems/activerecord-pedantmysql2-adapter-1.1.3/lib/active_record/connection_adapters/pedant_mysql2_adapter.rb:14:in `pedant_mysql2_connection'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:902:in `new_connection'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:946:in `checkout_new_connection'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:925:in `try_to_checkout_new_connection'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:886:in `acquire_connection'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:604:in `checkout'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:444:in `connection'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb:1134:in `retrieve_connection'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/connection_handling.rb:238:in `retrieve_connection'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/connection_handling.rb:206:in `connection'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/type.rb:51:in `current_adapter_name'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/type.rb:41:in `lookup'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/attributes.rb:250:in `block in load_schema!'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/attributes.rb:248:in `each'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/attributes.rb:248:in `load_schema!'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/attribute_decorators.rb:50:in `load_schema!'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/active_record/model_schema.rb:478:in `block in load_schema'
    from /usr/lib/ruby-shopify/ruby-shopify-2.6.4/lib/ruby/2.6.0/monitor.rb:230:in `mon_synchronize'
    from bundler/gems/rails-9c6bdb54a341/activerecord/lib/

What happened here is that a call to connection.lookup_cast_type_from_column raises and error, which leave the model with @columns_hash set, but not all attribute methods etc defined and @schema_loaded = false.

So effectively the model is only half-way initialized, but further calls to load_schema bail out early because @columns_hash is set.

Fix

So this is a very straightforward change, we just make sure that the condition used for bailing out early is one that prove everything was properly initialized.

This is not perfect though, because the model still end up in an inconsistent state, however it will recover on the next load_schema call. What we could do would be to add a rescue block to unset @column_hash and others. Let me know what you think about this.

Testing

I'd like to add a regression test for this, but to reproduce the error I think I need some stubbing helper, and I'm having a hard time to understand the ones available in Rails test suite.

@etiennebarrie @deuxpi @rafaelfranca @Edouard-chin @eileencodes @seejohnrun

[eileencodes]

@casperisfine do you know what change broke this or has it always been broken?

[casperisfine]

@eileencodes I'll do some git spelunking to try to figure that out, however we're seeing these errors since a fairly long time, but they are very infrequent, so it took a while to pinpoint.

[casperisfine]

Ok, so purely based on the source, I'd say the issue was introduced in 6a5a814

[casperisfine]

I just pushed a test that reproduce the error on master. However it seems like my fix is totally breaking the test suite with a StackLevelTooDeep.

I'll try to figure that out.

[casperisfine]

Ok, so the @columns_hash check can't be removed because load_schema can be called circularly, so we still need to bail out in this case.

So the next best thing is to reset the @column_hash if an exception was raised.

cc @matthewd since you seem to have worked in this area previously.