diff options
42 files changed, 907 insertions, 324 deletions
diff --git a/actionmailer/lib/action_mailer/base.rb b/actionmailer/lib/action_mailer/base.rb index 218b7a735a..4a5229fe18 100644 --- a/actionmailer/lib/action_mailer/base.rb +++ b/actionmailer/lib/action_mailer/base.rb @@ -257,8 +257,8 @@ module ActionMailer # <tt>ActionMailer::Base</tt> sets the following: # # * <tt>mime_version: "1.0"</tt> - # * <tt>charset: "UTF-8",</tt> - # * <tt>content_type: "text/plain",</tt> + # * <tt>charset: "UTF-8"</tt> + # * <tt>content_type: "text/plain"</tt> # * <tt>parts_order: [ "text/plain", "text/enriched", "text/html" ]</tt> # # <tt>parts_order</tt> and <tt>charset</tt> are not actually valid <tt>Mail::Message</tt> header fields, @@ -286,7 +286,7 @@ module ActionMailer # end # # Note that the proc is evaluated right at the start of the mail message generation, so if you - # set something in the defaults using a proc, and then set the same thing inside of your + # set something in the default using a proc, and then set the same thing inside of your # mailer method, it will get over written by the mailer method. # # It is also possible to set these default options that will be used in all mailers through @@ -319,8 +319,9 @@ module ActionMailer # callbacks in the same manner that you would use callbacks in classes that # inherit from <tt>ActionController::Base</tt>. # - # Note that unless you have a specific reason to do so, you should prefer using before_action - # rather than after_action in your Action Mailer classes so that headers are parsed properly. + # Note that unless you have a specific reason to do so, you should prefer + # using <tt>before_action</tt> rather than <tt>after_action</tt> in your + # Action Mailer classes so that headers are parsed properly. # # = Previewing emails # diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md index fbd6c5c747..c27ef2dc5f 100644 --- a/actionpack/CHANGELOG.md +++ b/actionpack/CHANGELOG.md @@ -1,4 +1,4 @@ -* Fix rake routes not showing the right format when +* Fix `rake routes` not showing the right format when nesting multiple routes. See #18373. diff --git a/actionpack/lib/action_dispatch/journey/routes.rb b/actionpack/lib/action_dispatch/journey/routes.rb index a6d1980db2..6325dfa3dd 100644 --- a/actionpack/lib/action_dispatch/journey/routes.rb +++ b/actionpack/lib/action_dispatch/journey/routes.rb @@ -16,6 +16,10 @@ module ActionDispatch @simulator = nil end + def empty? + routes.empty? + end + def length routes.length end diff --git a/actionpack/lib/action_dispatch/testing/integration.rb b/actionpack/lib/action_dispatch/testing/integration.rb index b1bd6ae6d5..f953429e96 100644 --- a/actionpack/lib/action_dispatch/testing/integration.rb +++ b/actionpack/lib/action_dispatch/testing/integration.rb @@ -584,7 +584,7 @@ module ActionDispatch # https!(false) # get "/articles/all" # assert_response :success - # assert assigns(:articles) + # assert_select 'h1', 'Articles' # end # end # @@ -623,7 +623,7 @@ module ActionDispatch # def browses_site # get "/products/all" # assert_response :success - # assert assigns(:products) + # assert_select 'h1', 'Products' # end # end # diff --git a/actionpack/test/dispatch/routing/route_set_test.rb b/actionpack/test/dispatch/routing/route_set_test.rb index fe52c50336..9327fe12c6 100644 --- a/actionpack/test/dispatch/routing/route_set_test.rb +++ b/actionpack/test/dispatch/routing/route_set_test.rb @@ -17,6 +17,16 @@ module ActionDispatch @set = RouteSet.new end + test "not being empty when route is added" do + assert empty? + + draw do + get 'foo', to: SimpleApp.new('foo#index') + end + + assert_not empty? + end + test "url helpers are added when route is added" do draw do get 'foo', to: SimpleApp.new('foo#index') @@ -136,6 +146,10 @@ module ActionDispatch def url_helpers @set.url_helpers end + + def empty? + @set.empty? + end end end end diff --git a/actionpack/test/journey/routes_test.rb b/actionpack/test/journey/routes_test.rb index b54d961f66..2a1c1c337e 100644 --- a/actionpack/test/journey/routes_test.rb +++ b/actionpack/test/journey/routes_test.rb @@ -14,9 +14,11 @@ module ActionDispatch requirements = { :hello => /world/ } routes.add_route nil, path, requirements, {:id => nil}, {} + assert_not routes.empty? assert_equal 1, routes.length routes.clear + assert routes.empty? assert_equal 0, routes.length end diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index 40da3dc19b..201e118971 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,4 +1,5 @@ -* Do not put partial name to local_assigns when rendering without an object or a collection. +* Do not put partial name to `local_assigns` when rendering without + an object or a collection. *Henrik Nygren* @@ -124,7 +125,7 @@ *Angelo Capilleri* -* Allow entries without a link tag in AtomFeedHelper. +* Allow entries without a link tag in `AtomFeedHelper`. *Daniel Gomez de Souza* diff --git a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb index b77c884e66..c8268e226e 100644 --- a/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb +++ b/actionview/lib/action_view/renderer/partial_renderer/collection_caching.rb @@ -6,7 +6,7 @@ module ActionView included do # Fallback cache store if Action View is used without Rails. - # Otherwise overriden in Railtie to use Rails.cache. + # Otherwise overridden in Railtie to use Rails.cache. mattr_accessor(:collection_cache) { ActiveSupport::Cache::MemoryStore.new } end diff --git a/activejob/CHANGELOG.md b/activejob/CHANGELOG.md index 74f4c2588e..aba386c18e 100644 --- a/activejob/CHANGELOG.md +++ b/activejob/CHANGELOG.md @@ -11,7 +11,7 @@ *y-yagi* -* A generated job now inherents from `app/jobs/application_job.rb` by default. +* A generated job now inherits from `app/jobs/application_job.rb` by default. *Jeroen van Baarsen* diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 3663a6faa4..5b09101bca 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -3,7 +3,8 @@ *Alex Robbin* -* Correctly pass MySQL options when using structure_dump or structure_load +* Correctly pass MySQL options when using `structure_dump` or + `structure_load`. Specifically, it fixes an issue when using SSL authentication. @@ -11,7 +12,7 @@ * Dump indexes in `create_table` instead of `add_index`. - If the adapter supports indexes in create table, generated SQL is + If the adapter supports indexes in `create_table`, generated SQL is slightly more efficient. *Ryuta Kamizono* diff --git a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb index 77e64a22be..6535121075 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb @@ -1,7 +1,6 @@ require 'thread' require 'thread_safe' require 'monitor' -require 'set' module ActiveRecord # Raised when a connection could not be obtained within the connection @@ -10,6 +9,12 @@ module ActiveRecord class ConnectionTimeoutError < ConnectionNotEstablished end + # Raised when a pool was unable to get ahold of all its connections + # to perform a "group" action such as +ConnectionPool#disconnect!+ + # or +ConnectionPool#clear_reloadable_connections!+. + class ExclusiveConnectionTimeoutError < ConnectionTimeoutError + end + module ConnectionAdapters # Connection pool base class for managing Active Record database # connections. @@ -63,6 +68,15 @@ module ActiveRecord # connection at the end of a thread or a thread dies unexpectedly. # Regardless of this setting, the Reaper will be invoked before every # blocking wait. (Default nil, which means don't schedule the Reaper). + # + #-- + # Synchronization policy: + # * all public methods can be called outside +synchronize+ + # * access to these i-vars needs to be in +synchronize+: + # * @connections + # * @now_connecting + # * private methods that require being called in a +synchronize+ blocks + # are now explicitly documented class ConnectionPool # Threadsafe, fair, FIFO queue. Meant to be used by ConnectionPool # with which it shares a Monitor. But could be a generic Queue. @@ -129,17 +143,15 @@ module ActiveRecord # - ConnectionTimeoutError if +timeout+ is given and no element # becomes available within +timeout+ seconds, def poll(timeout = nil) - synchronize do - if timeout - no_wait_poll || wait_poll(timeout) - else - no_wait_poll - end - end + synchronize { internal_poll(timeout) } end private + def internal_poll(timeout) + no_wait_poll || (timeout && wait_poll(timeout)) + end + def synchronize(&block) @lock.synchronize(&block) end @@ -193,6 +205,80 @@ module ActiveRecord end end + # Adds the ability to turn a basic fair FIFO queue into one + # biased to some thread. + module BiasableQueue # :nodoc: + class BiasedConditionVariable # :nodoc: + # semantics of condition variables guarantee that +broadcast+, +broadcast_on_biased+, + # +signal+ and +wait+ methods are only called while holding a lock + def initialize(lock, other_cond, preferred_thread) + @real_cond = lock.new_cond + @other_cond = other_cond + @preferred_thread = preferred_thread + @num_waiting_on_real_cond = 0 + end + + def broadcast + broadcast_on_biased + @other_cond.broadcast + end + + def broadcast_on_biased + @num_waiting_on_real_cond = 0 + @real_cond.broadcast + end + + def signal + if @num_waiting_on_real_cond > 0 + @num_waiting_on_real_cond -= 1 + @real_cond + else + @other_cond + end.signal + end + + def wait(timeout) + if Thread.current == @preferred_thread + @num_waiting_on_real_cond += 1 + @real_cond + else + @other_cond + end.wait(timeout) + end + end + + def with_a_bias_for(thread) + previous_cond = nil + new_cond = nil + synchronize do + previous_cond = @cond + @cond = new_cond = BiasedConditionVariable.new(@lock, @cond, thread) + end + yield + ensure + synchronize do + @cond = previous_cond if previous_cond + new_cond.broadcast_on_biased if new_cond # wake up any remaining sleepers + end + end + end + + # Connections must be leased while holding the main pool mutex. This is + # an internal subclass that also +.leases+ returned connections while + # still in queue's critical section (queue synchronizes with the same + # +@lock+ as the main pool) so that a returned connection is already + # leased and there is no need to re-enter synchronized block. + class ConnectionLeasingQueue < Queue # :nodoc: + include BiasableQueue + + private + def internal_poll(timeout) + conn = super + conn.lease if conn + conn + end + end + # Every +frequency+ seconds, the reaper will call +reap+ on +pool+. # A reaper instantiated with a nil frequency will never reap the # connection pool. @@ -241,56 +327,75 @@ module ActiveRecord # default max pool size to 5 @size = (spec.config[:pool] && spec.config[:pool].to_i) || 5 - # The cache of reserved connections mapped to threads - @reserved_connections = ThreadSafe::Cache.new(:initial_capacity => @size) + # The cache of threads mapped to reserved connections, the sole purpose + # of the cache is to speed-up +connection+ method, it is not the authoritative + # registry of which thread owns which connection, that is tracked by + # +connection.owner+ attr on each +connection+ instance. + # The invariant works like this: if there is mapping of +thread => conn+, + # then that +thread+ does indeed own that +conn+, however an absence of a such + # mapping does not mean that the +thread+ doesn't own the said connection, in + # that case +conn.owner+ attr should be consulted. + # Access and modification of +@thread_cached_conns+ does not require + # synchronization. + @thread_cached_conns = ThreadSafe::Cache.new(:initial_capacity => @size) @connections = [] @automatic_reconnect = true - @available = Queue.new self + # Connection pool allows for concurrent (outside the main `synchronize` section) + # establishment of new connections. This variable tracks the number of threads + # currently in the process of independently establishing connections to the DB. + @now_connecting = 0 + + # A boolean toggle that allows/disallows new connections. + @new_cons_enabled = true + + @available = ConnectionLeasingQueue.new self end # Retrieve the connection associated with the current thread, or call # #checkout to obtain one if necessary. # # #connection can be called any number of times; the connection is - # held in a hash keyed by the thread id. + # held in a cache keyed by a thread. def connection - # this is correctly done double-checked locking - # (ThreadSafe::Cache's lookups have volatile semantics) - @reserved_connections[current_connection_id] || synchronize do - @reserved_connections[current_connection_id] ||= checkout - end + @thread_cached_conns[connection_cache_key(Thread.current)] ||= checkout end # Is there an open connection that is being used for the current thread? + # + # This method only works for connections that have been abtained through + # #connection or #with_connection methods, connections obtained through + # #checkout will not be detected by #active_connection? def active_connection? - synchronize do - @reserved_connections.fetch(current_connection_id) { - return false - }.in_use? - end + @thread_cached_conns[connection_cache_key(Thread.current)] end # Signal that the thread is finished with the current connection. # #release_connection releases the connection-thread association # and returns the connection to the pool. - def release_connection(with_id = current_connection_id) - synchronize do - conn = @reserved_connections.delete(with_id) - checkin conn if conn + # + # This method only works for connections that have been obtained through + # #connection or #with_connection methods, connections obtained through + # #checkout will not be automatically released. + def release_connection(owner_thread = Thread.current) + if conn = @thread_cached_conns.delete(connection_cache_key(owner_thread)) + checkin conn end end - # If a connection already exists yield it to the block. If no connection + # If a connection obtained through #connection or #with_connection methods + # already exists yield it to the block. If no such connection # exists checkout a connection, yield it to the block, and checkin the # connection when finished. def with_connection - connection_id = current_connection_id - fresh_connection = true unless active_connection? - yield connection + unless conn = @thread_cached_conns[connection_cache_key(Thread.current)] + conn = connection + fresh_connection = true + end + yield conn ensure - release_connection(connection_id) if fresh_connection + release_connection if fresh_connection end # Returns true if a connection has already been opened. @@ -299,32 +404,81 @@ module ActiveRecord end # Disconnects all connections in the pool, and clears the pool. - def disconnect! - synchronize do - @reserved_connections.clear - @connections.each do |conn| - checkin conn - conn.disconnect! + # + # Raises: + # - +ExclusiveConnectionTimeoutError+ if unable to gain ownership of all + # connections in the pool within a timeout interval (default duration is + # +spec.config[:checkout_timeout] * 2+ seconds). + def disconnect(raise_on_acquisition_timeout = true) + with_exclusively_acquired_all_connections(raise_on_acquisition_timeout) do + synchronize do + @connections.each do |conn| + checkin conn + conn.disconnect! + end + @connections = [] + @available.clear end - @connections = [] - @available.clear end end - # Clears the cache which maps classes. - def clear_reloadable_connections! - synchronize do - @reserved_connections.clear - @connections.each do |conn| - checkin conn - conn.disconnect! if conn.requires_reloading? - end - @connections.delete_if(&:requires_reloading?) - @available.clear - @connections.each do |conn| - @available.add conn + # Disconnects all connections in the pool, and clears the pool. + # + # The pool first tries to gain ownership of all connections, if unable to + # do so within a timeout interval (default duration is + # +spec.config[:checkout_timeout] * 2+ seconds), the pool is forcefully + # disconneted wihout any regard for other connection owning threads. + def disconnect! + disconnect(false) + end + + # Clears the cache which maps classes and re-connects connections that + # require reloading. + # + # Raises: + # - +ExclusiveConnectionTimeoutError+ if unable to gain ownership of all + # connections in the pool within a timeout interval (default duration is + # +spec.config[:checkout_timeout] * 2+ seconds). + def clear_reloadable_connections(raise_on_acquisition_timeout = true) + num_new_conns_required = 0 + + with_exclusively_acquired_all_connections(raise_on_acquisition_timeout) do + synchronize do + @connections.each do |conn| + checkin conn + conn.disconnect! if conn.requires_reloading? + end + @connections.delete_if(&:requires_reloading?) + + @available.clear + + if @connections.size < @size + # because of the pruning done by this method, we might be running + # low on connections, while threads stuck in queue are helpless + # (not being able to establish new connections for themselves), + # see also more detailed explanation in +remove+ + num_new_conns_required = num_waiting_in_queue - @connections.size + end + + @connections.each do |conn| + @available.add conn + end end end + + bulk_make_new_connections(num_new_conns_required) if num_new_conns_required > 0 + end + + # Clears the cache which maps classes and re-connects connections that + # require reloading. + # + # The pool first tries to gain ownership of all connections, if unable to + # do so within a timeout interval (default duration is + # +spec.config[:checkout_timeout] * 2+ seconds), the pool forcefully + # clears the cache and reloads connections without any regard for other + # connection owning threads. + def clear_reloadable_connections! + clear_reloadable_connections(false) end # Check-out a database connection from the pool, indicating that you want @@ -341,12 +495,8 @@ module ActiveRecord # # Raises: # - ConnectionTimeoutError: no connection can be obtained from the pool. - def checkout - synchronize do - conn = acquire_connection - conn.lease - checkout_and_verify(conn) - end + def checkout(checkout_timeout = @checkout_timeout) + checkout_and_verify(acquire_connection(checkout_timeout)) end # Check-in a database connection back into the pool, indicating that you @@ -356,14 +506,12 @@ module ActiveRecord # calling +checkout+ on this pool. def checkin(conn) synchronize do - owner = conn.owner + remove_connection_from_thread_cache conn conn.run_callbacks :checkin do conn.expire end - release conn, owner - @available.add conn end end @@ -371,14 +519,32 @@ module ActiveRecord # Remove a connection from the connection pool. The connection will # remain open and active but will no longer be managed by this pool. def remove(conn) + needs_new_connection = false + synchronize do + remove_connection_from_thread_cache conn + @connections.delete conn @available.delete conn - release conn, conn.owner - - @available.add checkout_new_connection if @available.any_waiting? + # @available.any_waiting? => true means that prior to removing this + # conn, the pool was at its max size (@connections.size == @size) + # this would mean that any threads stuck waiting in the queue wouldn't + # know they could checkout_new_connection, so let's do it for them. + # Because condition-wait loop is encapsulated in the Queue class + # (that in turn is oblivious to ConnectionPool implementation), threads + # that are "stuck" there are helpless, they have no way of creating + # new connections and are completely reliant on us feeding available + # connections into the Queue. + needs_new_connection = @available.any_waiting? end + + # This is intentionally done outside of the synchronized section as we + # would like not to hold the main mutex while checking out new connections, + # thus there is some chance that needs_new_connection information is now + # stale, we can live with that (bulk_make_new_connections will make + # sure not to exceed the pool's @size limit). + bulk_make_new_connections(1) if needs_new_connection end # Recover lost connections for the pool. A lost connection can occur if @@ -403,7 +569,118 @@ module ActiveRecord end end + def num_waiting_in_queue # :nodoc: + @available.num_waiting + end + private + #-- + # this is unfortunately not concurrent + def bulk_make_new_connections(num_new_conns_needed) + num_new_conns_needed.times do + # try_to_checkout_new_connection will not exceed pool's @size limit + if new_conn = try_to_checkout_new_connection + # make the new_conn available to the starving threads stuck @available Queue + checkin(new_conn) + end + end + end + + #-- + # From the discussion on Github: + # https://github.com/rails/rails/pull/14938#commitcomment-6601951 + # This hook-in method allows for easier monkey-patching fixes needed by + # JRuby users that use Fibers. + def connection_cache_key(thread) + thread + end + + # Take control of all existing connections so a "group" action such as + # reload/disconnect can be performed safely. It is no longer enough to + # wrap it in +synchronize+ because some pool's actions are allowed + # to be performed outside of the main +synchronize+ block. + def with_exclusively_acquired_all_connections(raise_on_acquisition_timeout = true) + with_new_connections_blocked do + attempt_to_checkout_all_existing_connections(raise_on_acquisition_timeout) + yield + end + end + + def attempt_to_checkout_all_existing_connections(raise_on_acquisition_timeout = true) + collected_conns = synchronize do + # account for our own connections + @connections.select {|conn| conn.owner == Thread.current} + end + + newly_checked_out = [] + timeout_time = Time.now + (@checkout_timeout * 2) + + @available.with_a_bias_for(Thread.current) do + while true + synchronize do + return if collected_conns.size == @connections.size && @now_connecting == 0 + remaining_timeout = timeout_time - Time.now + remaining_timeout = 0 if remaining_timeout < 0 + conn = checkout_for_exclusive_access(remaining_timeout) + collected_conns << conn + newly_checked_out << conn + end + end + end + rescue ExclusiveConnectionTimeoutError + # `raise_on_acquisition_timeout == false` means we are directed to ignore any + # timeouts and are expected to just give up: we've obtained as many connections + # as possible, note that in a case like that we don't return any of the + # `newly_checked_out` connections. + + if raise_on_acquisition_timeout + release_newly_checked_out = true + raise + end + rescue Exception # if something else went wrong + # this can't be a "naked" rescue, because we have should return conns + # even for non-StandardErrors + release_newly_checked_out = true + raise + ensure + if release_newly_checked_out && newly_checked_out + # releasing only those conns that were checked out in this method, conns + # checked outside this method (before it was called) are not for us to release + newly_checked_out.each {|conn| checkin(conn)} + end + end + + #-- + # Must be called in a synchronize block. + def checkout_for_exclusive_access(checkout_timeout) + checkout(checkout_timeout) + rescue ConnectionTimeoutError + # this block can't be easily moved into attempt_to_checkout_all_existing_connections's + # rescue block, because doing so would put it outside of synchronize section, without + # being in a critical section thread_report might become inaccurate + msg = "could not obtain ownership of all database connections in #{checkout_timeout} seconds" + + thread_report = [] + @connections.each do |conn| + unless conn.owner == Thread.current + thread_report << "#{conn} is owned by #{conn.owner}" + end + end + + msg << " (#{thread_report.join(', ')})" if thread_report.any? + + raise ExclusiveConnectionTimeoutError, msg + end + + def with_new_connections_blocked + previous_value = nil + synchronize do + previous_value, @new_cons_enabled = @new_cons_enabled, false + end + yield + ensure + synchronize { @new_cons_enabled = previous_value } + end # Acquire a connection by one of 1) immediately removing one # from the queue of available connections, 2) creating a new @@ -412,24 +689,31 @@ module ActiveRecord # # Raises: # - ConnectionTimeoutError if a connection could not be acquired - def acquire_connection - if conn = @available.poll + # + #-- + # Implementation detail: the connection returned by +acquire_connection+ + # will already be "+connection.lease+ -ed" to the current thread. + def acquire_connection(checkout_timeout) + # NOTE: we rely on `@available.poll` and `try_to_checkout_new_connection` to + # `conn.lease` the returned connection (and to do this in a `synchronized` + # section), this is not the cleanest implementation, as ideally we would + # `synchronize { conn.lease }` in this method, but by leaving it to `@available.poll` + # and `try_to_checkout_new_connection` we can piggyback on `synchronize` sections + # of the said methods and avoid an additional `synchronize` overhead. + if conn = @available.poll || try_to_checkout_new_connection conn - elsif @connections.size < @size - checkout_new_connection else reap - @available.poll(@checkout_timeout) + @available.poll(checkout_timeout) end end - def release(conn, owner) - thread_id = owner.object_id - - if @reserved_connections[thread_id] == conn - @reserved_connections.delete thread_id - end + #-- + # if owner_thread param is omitted, this must be called in synchronize block + def remove_connection_from_thread_cache(conn, owner_thread = conn.owner) + @thread_cached_conns.delete_pair(connection_cache_key(owner_thread), conn) end + alias_method :release, :remove_connection_from_thread_cache def new_connection Base.send(spec.adapter_method, spec.config).tap do |conn| @@ -437,17 +721,46 @@ module ActiveRecord end end - def current_connection_id #:nodoc: - Base.connection_id ||= Thread.current.object_id + # If the pool is not at a +@size+ limit, establish new connection. Connecting + # to the DB is done outside main synchronized section. + #-- + # Implementation constraint: a newly established connection returned by this + # method must be in the +.leased+ state. + def try_to_checkout_new_connection + # first in synchronized section check if establishing new conns is allowed + # and increment @now_connecting, to prevent overstepping this pool's @size + # constraint + do_checkout = synchronize do + if @new_cons_enabled && (@connections.size + @now_connecting) < @size + @now_connecting += 1 + end + end + if do_checkout + begin + # if successfully incremented @now_connecting establish new connection + # outside of synchronized section + conn = checkout_new_connection + ensure + synchronize do + if conn + adopt_connection(conn) + # returned conn needs to be already leased + conn.lease + end + @now_connecting -= 1 + end + end + end + end + + def adopt_connection(conn) + conn.pool = self + @connections << conn end def checkout_new_connection raise ConnectionNotEstablished unless @automatic_reconnect - - c = new_connection - c.pool = self - @connections << c - c + new_connection end def checkout_and_verify(c) @@ -620,7 +933,9 @@ module ActiveRecord # A connection was established in an ancestor process that must have # subsequently forked. We can't reuse the connection, but we can copy # the specification and establish a new connection with it. - establish_connection owner, ancestor_pool.spec + establish_connection(owner, ancestor_pool.spec).tap do |pool| + pool.schema_cache = ancestor_pool.schema_cache if ancestor_pool.schema_cache + end else owner_to_pool[owner.name] = nil end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb index 0ccf0c498b..429445540d 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_definitions.rb @@ -1,5 +1,4 @@ require 'date' -require 'set' require 'bigdecimal' require 'bigdecimal/util' @@ -403,17 +402,12 @@ module ActiveRecord column(:updated_at, :datetime, options) end - # Adds a reference. Optionally adds a +type+ column, if the - # +:polymorphic+ option is provided. +references+ and +belongs_to+ - # are interchangeable. The reference column will be an +integer+ by default, - # the +:type+ option can be used to specify a different type. A foreign - # key will be created if the +:foreign_key+ option is passed. + # Adds a reference. # # t.references(:user) - # t.references(:user, type: "string") - # t.belongs_to(:supplier, polymorphic: true) + # t.belongs_to(:supplier, foreign_key: true) # - # See SchemaStatements#add_reference + # See SchemaStatements#add_reference for details of the options you can use. def references(*args, **options) args.each do |col| ReferenceDefinition.new(col, **options).add_to(self) @@ -647,15 +641,12 @@ module ActiveRecord @base.rename_column(name, column_name, new_column_name) end - # Adds a reference. Optionally adds a +type+ column, if - # <tt>:polymorphic</tt> option is provided. + # Adds a reference. # # t.references(:user) - # t.references(:user, type: "string") - # t.belongs_to(:supplier, polymorphic: true) # t.belongs_to(:supplier, foreign_key: true) # - # See SchemaStatements#add_reference + # See SchemaStatements#add_reference for details of the options you can use. def references(*args) options = args.extract_options! args.each do |ref_name| diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb index deb014ad46..b944a8631c 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_dumper.rb @@ -27,10 +27,17 @@ module ActiveRecord spec[:type] = schema_type(column) spec[:null] = 'false' unless column.null - limit = column.limit || native_database_types[column.type][:limit] - spec[:limit] = limit.inspect if limit - spec[:precision] = column.precision.inspect if column.precision - spec[:scale] = column.scale.inspect if column.scale + if limit = schema_limit(column) + spec[:limit] = limit + end + + if precision = schema_precision(column) + spec[:precision] = precision + end + + if scale = schema_scale(column) + spec[:scale] = scale + end default = schema_default(column) if column.has_default? spec[:default] = default unless default.nil? @@ -53,6 +60,19 @@ module ActiveRecord column.type.to_s end + def schema_limit(column) + limit = column.limit || native_database_types[column.type][:limit] + limit.inspect if limit + end + + def schema_precision(column) + column.precision.inspect if column.precision + end + + def schema_scale(column) + column.scale.inspect if column.scale + end + def schema_default(column) type = lookup_cast_type_from_column(column) default = type.deserialize(column.default) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb index 9004d86b04..ed19819d63 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -649,11 +649,21 @@ module ActiveRecord indexes(table_name).detect { |i| i.name == index_name } end - # Adds a reference. Optionally adds a +type+ column, if <tt>:polymorphic</tt> option is provided. - # The reference column is an +integer+ by default, the <tt>:type</tt> option can be used to specify - # a different type. + # Adds a reference. The reference column is an integer by default, + # the <tt>:type</tt> option can be used to specify a different type. + # Optionally adds a +_type+ column, if <tt>:polymorphic</tt> option is provided. # <tt>add_reference</tt> and <tt>add_belongs_to</tt> are acceptable. # + # The +options+ hash can include the following keys: + # [<tt>:type</tt>] + # The reference column type. Defaults to +:integer+. + # [<tt>:index</tt>] + # Add an appropriate index. Defaults to false. + # [<tt>:foreign_key</tt>] + # Add an appropriate foreign key. Defaults to false. + # [<tt>:polymorphic</tt>] + # Wether an additional +_type+ column should be added. Defaults to false. + # # ====== Create a user_id integer column # # add_reference(:products, :user) @@ -662,10 +672,6 @@ module ActiveRecord # # add_reference(:products, :user, type: :string) # - # ====== Create a supplier_id and supplier_type columns - # - # add_belongs_to(:products, :supplier, polymorphic: true) - # # ====== Create supplier_id, supplier_type columns and appropriate index # # add_reference(:products, :supplier, polymorphic: true, index: true) diff --git a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb index 0705c22a8c..60655b673c 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb @@ -7,7 +7,6 @@ require 'active_record/connection_adapters/schema_cache' require 'active_record/connection_adapters/sql_type_metadata' require 'active_record/connection_adapters/abstract/schema_dumper' require 'active_record/connection_adapters/abstract/schema_creation' -require 'monitor' require 'arel/collectors/bind' require 'arel/collectors/sql_string' @@ -70,7 +69,6 @@ module ActiveRecord include DatabaseLimits include QueryCache include ActiveSupport::Callbacks - include MonitorMixin include ColumnDumper SIMPLE_INT = /\A\d+\z/ @@ -141,12 +139,20 @@ module ActiveRecord SchemaCreation.new self end + # this method must only be called while holding connection pool's mutex def lease - synchronize do - unless in_use? - @owner = Thread.current + if in_use? + msg = 'Cannot lease connection, ' + if @owner == Thread.current + msg << 'it is already leased by the current thread.' + else + msg << "it is already in use by a different thread: #{@owner}. " << + "Current thread: #{Thread.current}." end + raise ActiveRecordError, msg end + + @owner = Thread.current end def schema_cache=(cache) @@ -154,6 +160,7 @@ module ActiveRecord @schema_cache = cache end + # this method must only be called while holding connection pool's mutex def expire @owner = nil end diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index 0862c7678e..8776d0adf4 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -122,11 +122,14 @@ module ActiveRecord spec end - def prepare_column_options(column) - spec = super - spec.delete(:precision) if /time/ === column.sql_type && column.precision == 0 - spec.delete(:limit) if :boolean === column.type - spec + private + + def schema_limit(column) + super unless column.type == :boolean + end + + def schema_precision(column) + super unless /time/ === column.sql_type && column.precision == 0 end def schema_collation(column) @@ -136,7 +139,8 @@ module ActiveRecord column.collation.inspect if column.collation != @collation_cache[table_name] end end - private :schema_collation + + public class Column < ConnectionAdapters::Column # :nodoc: delegate :strict, :extra, to: :sql_type_metadata, allow_nil: true diff --git a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb index 18febf66b4..2ae462d773 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb @@ -71,34 +71,10 @@ module ActiveRecord ADAPTER_NAME = 'MySQL'.freeze class StatementPool < ConnectionAdapters::StatementPool - def initialize(connection, max = 1000) - super - @cache = Hash.new { |h,pid| h[pid] = {} } - end - - def each(&block); cache.each(&block); end - def key?(key); cache.key?(key); end - def [](key); cache[key]; end - def length; cache.length; end - def delete(key); cache.delete(key); end - - def []=(sql, key) - while @max <= cache.size - cache.shift.last[:stmt].close - end - cache[sql] = key - end - - def clear - cache.each_value do |hash| - hash[:stmt].close - end - cache.clear - end - private - def cache - @cache[Process.pid] + + def dealloc(stmt) + stmt[:stmt].close end end @@ -416,8 +392,11 @@ module ActiveRecord # place when an error occurs. To support older MySQL versions, we # need to close the statement and delete the statement from the # cache. - stmt.close - @statements.delete sql + if binds.empty? + stmt.close + else + @statements.delete sql + end raise e end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb index be13ead120..bfa03fa136 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/column.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/column.rb @@ -8,7 +8,8 @@ module ActiveRecord def serial? return unless default_function - %r{\Anextval\('(?<table_name>.+)_#{name}_seq'::regclass\)\z} === default_function + table_name = @table_name || '(?<table_name>.+)' + %r{\Anextval\('"?#{table_name}_#{name}_seq"?'::regclass\)\z} === default_function end end end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb index 412999f0fe..595c635fc0 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb @@ -70,11 +70,7 @@ module ActiveRecord # Returns the list of all tables in the schema search path or a specified schema. def tables(name = nil) - select_values(<<-SQL, 'SCHEMA') - SELECT tablename - FROM pg_tables - WHERE schemaname = ANY (current_schemas(false)) - SQL + select_values("SELECT tablename FROM pg_tables WHERE schemaname = ANY(current_schemas(false))", 'SCHEMA') end # Returns true if table exists. @@ -100,11 +96,7 @@ module ActiveRecord # Returns true if schema exists. def schema_exists?(name) - select_value(<<-SQL, 'SCHEMA').to_i > 0 - SELECT COUNT(*) - FROM pg_namespace - WHERE nspname = '#{name}' - SQL + select_value("SELECT COUNT(*) FROM pg_namespace WHERE nspname = '#{name}'", 'SCHEMA').to_i > 0 end # Verifies existence of an index with a given name. @@ -192,24 +184,17 @@ module ActiveRecord # Returns the current database encoding format. def encoding - select_value(<<-end_sql, 'SCHEMA') - SELECT pg_encoding_to_char(pg_database.encoding) FROM pg_database - WHERE pg_database.datname LIKE '#{current_database}' - end_sql + select_value("SELECT pg_encoding_to_char(encoding) FROM pg_database WHERE datname LIKE '#{current_database}'", 'SCHEMA') end # Returns the current database collation. def collation - select_value(<<-end_sql, 'SCHEMA') - SELECT pg_database.datcollate FROM pg_database WHERE pg_database.datname LIKE '#{current_database}' - end_sql + select_value("SELECT datcollate FROM pg_database WHERE datname LIKE '#{current_database}'", 'SCHEMA') end # Returns the current database ctype. def ctype - select_value(<<-end_sql, 'SCHEMA') - SELECT pg_database.datctype FROM pg_database WHERE pg_database.datname LIKE '#{current_database}' - end_sql + select_value("SELECT datctype FROM pg_database WHERE datname LIKE '#{current_database}'", 'SCHEMA') end # Returns an array of schema names. diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 2b33a5b9cb..ec8bfac455 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -211,44 +211,18 @@ module ActiveRecord def initialize(connection, max) super @counter = 0 - @cache = Hash.new { |h,pid| h[pid] = {} } end - def each(&block); cache.each(&block); end - def key?(key); cache.key?(key); end - def [](key); cache[key]; end - def length; cache.length; end - def next_key "a#{@counter + 1}" end def []=(sql, key) - while @max <= cache.size - dealloc(cache.shift.last) - end - @counter += 1 - cache[sql] = key - end - - def clear - cache.each_value do |stmt_key| - dealloc stmt_key - end - cache.clear - end - - def delete(sql_key) - dealloc cache[sql_key] - cache.delete sql_key + super.tap { @counter += 1 } end private - def cache - @cache[Process.pid] - end - def dealloc(key) @connection.query "DEALLOCATE #{key}" if connection_active? end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index 3186769510..2b2050d7af 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -78,37 +78,10 @@ module ActiveRecord end class StatementPool < ConnectionAdapters::StatementPool - def initialize(connection, max) - super - @cache = Hash.new { |h,pid| h[pid] = {} } - end - - def each(&block); cache.each(&block); end - def key?(key); cache.key?(key); end - def [](key); cache[key]; end - def length; cache.length; end - - def []=(sql, key) - while @max <= cache.size - dealloc(cache.shift.last[:stmt]) - end - cache[sql] = key - end - - def clear - cache.each_value do |hash| - dealloc hash[:stmt] - end - cache.clear - end - private - def cache - @cache[$$] - end def dealloc(stmt) - stmt.close unless stmt.closed? + stmt[:stmt].close unless stmt[:stmt].closed? end end diff --git a/activerecord/lib/active_record/connection_adapters/statement_pool.rb b/activerecord/lib/active_record/connection_adapters/statement_pool.rb index c6b1bc8b5b..82e9ef3d3d 100644 --- a/activerecord/lib/active_record/connection_adapters/statement_pool.rb +++ b/activerecord/lib/active_record/connection_adapters/statement_pool.rb @@ -4,35 +4,53 @@ module ActiveRecord include Enumerable def initialize(connection, max = 1000) + @cache = Hash.new { |h,pid| h[pid] = {} } @connection = connection @max = max end - def each - raise NotImplementedError + def each(&block) + cache.each(&block) end def key?(key) - raise NotImplementedError + cache.key?(key) end def [](key) - raise NotImplementedError + cache[key] end def length - raise NotImplementedError + cache.length end - def []=(sql, key) - raise NotImplementedError + def []=(sql, stmt) + while @max <= cache.size + dealloc(cache.shift.last) + end + cache[sql] = stmt end def clear - raise NotImplementedError + cache.each_value do |stmt| + dealloc stmt + end + cache.clear end def delete(key) + dealloc cache[key] + cache.delete(key) + end + + private + + def cache + @cache[Process.pid] + end + + def dealloc(stmt) raise NotImplementedError end end diff --git a/activerecord/lib/active_record/connection_handling.rb b/activerecord/lib/active_record/connection_handling.rb index 24f5849e45..d6b661ff76 100644 --- a/activerecord/lib/active_record/connection_handling.rb +++ b/activerecord/lib/active_record/connection_handling.rb @@ -88,7 +88,7 @@ module ActiveRecord end def connection_id - ActiveRecord::RuntimeRegistry.connection_id + ActiveRecord::RuntimeRegistry.connection_id ||= Thread.current.object_id end def connection_id=(connection_id) diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 90e37e80d2..07e0f134aa 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -166,6 +166,11 @@ module ActiveRecord # member.posts.first.title # => '[UPDATED] An, as of yet, undisclosed awesome Ruby documentation browser!' # member.posts.second.title # => '[UPDATED] other post' # + # However, the above applies if the parent model is being updated as well. + # For example, If you wanted to create a +member+ named _joe_ and wanted to + # update the +posts+ at the same time, that would give an + # ActiveRecord::RecordNotFound error. + # # By default the associated records are protected from being destroyed. If # you want to destroy any of the associated records through the attributes # hash, you have to enable it first using the <tt>:allow_destroy</tt> diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb index 65b607ff1c..c3054f1fe9 100644 --- a/activerecord/lib/active_record/relation/merger.rb +++ b/activerecord/lib/active_record/relation/merger.rb @@ -1,5 +1,4 @@ require 'active_support/core_ext/hash/keys' -require "set" module ActiveRecord class Relation diff --git a/activerecord/lib/active_record/type/integer.rb b/activerecord/lib/active_record/type/integer.rb index 2a1b04ac7f..c5040c6d3b 100644 --- a/activerecord/lib/active_record/type/integer.rb +++ b/activerecord/lib/active_record/type/integer.rb @@ -46,18 +46,21 @@ module ActiveRecord def ensure_in_range(value) unless range.cover?(value) - raise RangeError, "#{value} is out of range for #{self.class} with limit #{limit || DEFAULT_LIMIT}" + raise RangeError, "#{value} is out of range for #{self.class} with limit #{_limit}" end end def max_value - limit = self.limit || DEFAULT_LIMIT - 1 << (limit * 8 - 1) # 8 bits per byte with one bit for sign + 1 << (_limit * 8 - 1) # 8 bits per byte with one bit for sign end def min_value -max_value end + + def _limit + self.limit || DEFAULT_LIMIT + end end end end diff --git a/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb b/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb index 662e19f35e..580568c8ac 100644 --- a/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb +++ b/activerecord/test/cases/connection_adapters/adapter_leasing_test.rb @@ -6,7 +6,7 @@ module ActiveRecord class Pool < ConnectionPool def insert_connection_for_test!(c) synchronize do - @connections << c + adopt_connection(c) @available.add c end end @@ -24,7 +24,9 @@ module ActiveRecord def test_lease_twice assert @adapter.lease, 'should lease adapter' - assert_not @adapter.lease, 'should not lease adapter' + assert_raises(ActiveRecordError) do + @adapter.lease + end end def test_expire_mutates_in_use diff --git a/activerecord/test/cases/connection_adapters/connection_handler_test.rb b/activerecord/test/cases/connection_adapters/connection_handler_test.rb index b72f8ca88c..9b1865e8bb 100644 --- a/activerecord/test/cases/connection_adapters/connection_handler_test.rb +++ b/activerecord/test/cases/connection_adapters/connection_handler_test.rb @@ -46,6 +46,52 @@ module ActiveRecord def test_connection_pools assert_equal([@pool], @handler.connection_pools) end + + if Process.respond_to?(:fork) + def test_connection_pool_per_pid + object_id = ActiveRecord::Base.connection.object_id + + rd, wr = IO.pipe + rd.binmode + wr.binmode + + pid = fork { + rd.close + wr.write Marshal.dump ActiveRecord::Base.connection.object_id + wr.close + exit! + } + + wr.close + + Process.waitpid pid + assert_not_equal object_id, Marshal.load(rd.read) + rd.close + end + + def test_retrieve_connection_pool_copies_schema_cache_from_ancestor_pool + @pool.schema_cache = @pool.connection.schema_cache + @pool.schema_cache.add('posts') + + rd, wr = IO.pipe + rd.binmode + wr.binmode + + pid = fork { + rd.close + pool = @handler.retrieve_connection_pool(@klass) + wr.write Marshal.dump pool.schema_cache.size + wr.close + exit! + } + + wr.close + + Process.waitpid pid + assert_equal @pool.schema_cache.size, Marshal.load(rd.read) + rd.close + end + end end end end diff --git a/activerecord/test/cases/connection_management_test.rb b/activerecord/test/cases/connection_management_test.rb index bab624b78a..dff6ea0fb0 100644 --- a/activerecord/test/cases/connection_management_test.rb +++ b/activerecord/test/cases/connection_management_test.rb @@ -26,29 +26,6 @@ module ActiveRecord assert ActiveRecord::Base.connection_handler.active_connections? end - if Process.respond_to?(:fork) - def test_connection_pool_per_pid - object_id = ActiveRecord::Base.connection.object_id - - rd, wr = IO.pipe - rd.binmode - wr.binmode - - pid = fork { - rd.close - wr.write Marshal.dump ActiveRecord::Base.connection.object_id - wr.close - exit! - } - - wr.close - - Process.waitpid pid - assert_not_equal object_id, Marshal.load(rd.read) - rd.close - end - end - def test_app_delegation manager = ConnectionManagement.new(@app) diff --git a/activerecord/test/cases/connection_pool_test.rb b/activerecord/test/cases/connection_pool_test.rb index f5928814a3..c905772193 100644 --- a/activerecord/test/cases/connection_pool_test.rb +++ b/activerecord/test/cases/connection_pool_test.rb @@ -100,7 +100,7 @@ module ActiveRecord t = Thread.new { @pool.checkout } # make sure our thread is in the timeout section - Thread.pass until t.status == "sleep" + Thread.pass until @pool.num_waiting_in_queue == 1 connection = cs.first connection.close @@ -112,7 +112,7 @@ module ActiveRecord t = Thread.new { @pool.checkout } # make sure our thread is in the timeout section - Thread.pass until t.status == "sleep" + Thread.pass until @pool.num_waiting_in_queue == 1 connection = cs.first @pool.remove connection @@ -234,7 +234,7 @@ module ActiveRecord mutex.synchronize { errors << e } end } - Thread.pass until t.status == "sleep" + Thread.pass until @pool.num_waiting_in_queue == i t end @@ -271,7 +271,7 @@ module ActiveRecord mutex.synchronize { errors << e } end } - Thread.pass until t.status == "sleep" + Thread.pass until @pool.num_waiting_in_queue == i t end @@ -356,6 +356,170 @@ module ActiveRecord pool.checkin connection end + + def test_concurrent_connection_establishment + assert_operator @pool.connections.size, :<=, 1 + + all_threads_in_new_connection = ActiveSupport::Concurrency::Latch.new(@pool.size - @pool.connections.size) + all_go = ActiveSupport::Concurrency::Latch.new + + @pool.singleton_class.class_eval do + define_method(:new_connection) do + all_threads_in_new_connection.release + all_go.await + super() + end + end + + connecting_threads = [] + @pool.size.times do + connecting_threads << Thread.new { @pool.checkout } + end + + begin + Timeout.timeout(5) do + # the kernel of the whole test is here, everything else is just scaffolding, + # this latch will not be released unless conn. pool allows for concurrent + # connection creation + all_threads_in_new_connection.await + end + rescue Timeout::Error + flunk 'pool unable to establish connections concurrently or implementation has ' << + 'changed, this test then needs to patch a different :new_connection method' + ensure + # clean up the threads + all_go.release + connecting_threads.map(&:join) + end + end + + def test_non_bang_disconnect_and_clear_reloadable_connections_throw_exception_if_threads_dont_return_their_conns + @pool.checkout_timeout = 0.001 # no need to delay test suite by waiting the whole full default timeout + [:disconnect, :clear_reloadable_connections].each do |group_action_method| + @pool.with_connection do |connection| + assert_raises(ExclusiveConnectionTimeoutError) do + Thread.new { @pool.send(group_action_method) }.join + end + end + end + end + + def test_disconnect_and_clear_reloadable_connections_attempt_to_wait_for_threads_to_return_their_conns + [:disconnect, :disconnect!, :clear_reloadable_connections, :clear_reloadable_connections!].each do |group_action_method| + begin + thread = timed_join_result = nil + @pool.with_connection do |connection| + thread = Thread.new { @pool.send(group_action_method) } + + # give the other `thread` some time to get stuck in `group_action_method` + timed_join_result = thread.join(0.3) + # thread.join # => `nil` means the other thread hasn't finished running and is still waiting for us to + # release our connection + assert_nil timed_join_result + + # assert that since this is within default timeout our connection hasn't been forcefully taken away from us + assert @pool.active_connection? + end + ensure + thread.join if thread && !timed_join_result # clean up the other thread + end + end + end + + def test_bang_versions_of_disconnect_and_clear_reloadable_connections_if_unable_to_aquire_all_connections_proceed_anyway + @pool.checkout_timeout = 0.001 # no need to delay test suite by waiting the whole full default timeout + [:disconnect!, :clear_reloadable_connections!].each do |group_action_method| + @pool.with_connection do |connection| + Thread.new { @pool.send(group_action_method) }.join + # assert connection has been forcefully taken away from us + assert_not @pool.active_connection? + end + end + end + + def test_disconnect_and_clear_reloadable_connections_are_able_to_preempt_other_waiting_threads + with_single_connection_pool do |pool| + [:disconnect, :disconnect!, :clear_reloadable_connections, :clear_reloadable_connections!].each do |group_action_method| + conn = pool.connection # drain the only available connection + second_thread_done = ActiveSupport::Concurrency::Latch.new + + # create a first_thread and let it get into the FIFO queue first + first_thread = Thread.new do + pool.with_connection { second_thread_done.await } + end + + # wait for first_thread to get in queue + Thread.pass until pool.num_waiting_in_queue == 1 + + # create a different, later thread, that will attempt to do a "group action", + # but because of the group action semantics it should be able to preempt the + # first_thread when a connection is made available + second_thread = Thread.new do + pool.send(group_action_method) + second_thread_done.release + end + + # wait for second_thread to get in queue + Thread.pass until pool.num_waiting_in_queue == 2 + + # return the only available connection + pool.checkin(conn) + + # if the second_thread is not able to preempt the first_thread, + # they will temporarily (until either of them timeouts with ConnectionTimeoutError) + # deadlock and a join(2) timeout will be reached + failed = true unless second_thread.join(2) + + #--- post test clean up start + second_thread_done.release if failed + + # after `pool.disconnect()` the first thread will be left stuck in queue, no need to wait for + # it to timeout with ConnectionTimeoutError + if (group_action_method == :disconnect || group_action_method == :disconnect!) && pool.num_waiting_in_queue > 0 + pool.with_connection {} # create a new connection in case there are threads still stuck in a queue + end + + first_thread.join + second_thread.join + #--- post test clean up end + + flunk "#{group_action_method} is not able to preempt other waiting threads" if failed + end + end + end + + def test_clear_reloadable_connections_creates_new_connections_for_waiting_threads_if_necessary + with_single_connection_pool do |pool| + conn = pool.connection # drain the only available connection + def conn.requires_reloading? # make sure it gets removed from the pool by clear_reloadable_connections + true + end + + stuck_thread = Thread.new do + pool.with_connection {} + end + + # wait for stuck_thread to get in queue + Thread.pass until pool.num_waiting_in_queue == 1 + + pool.clear_reloadable_connections + + unless stuck_thread.join(2) + flunk 'clear_reloadable_connections must not let other connection waiting threads get stuck in queue' + end + + assert_equal 0, pool.num_waiting_in_queue + end + end + + private + def with_single_connection_pool + one_conn_spec = ActiveRecord::Base.connection_pool.spec.dup + one_conn_spec.config[:pool] = 1 # this is safe to do, because .dupped ConnectionSpecification also auto-dups its config + yield(pool = ConnectionPool.new(one_conn_spec)) + ensure + pool.disconnect! if pool + end end end end diff --git a/activerecord/test/cases/primary_keys_test.rb b/activerecord/test/cases/primary_keys_test.rb index 83be9a75d8..b8433f0bba 100644 --- a/activerecord/test/cases/primary_keys_test.rb +++ b/activerecord/test/cases/primary_keys_test.rb @@ -175,6 +175,14 @@ class PrimaryKeysTest < ActiveRecord::TestCase dashboard = Dashboard.first assert_equal '2', dashboard.id end + + if current_adapter?(:PostgreSQLAdapter) + def test_serial_with_quoted_sequence_name + column = MixedCaseMonkey.columns_hash[MixedCaseMonkey.primary_key] + assert_equal "nextval('\"mixed_case_monkeys_monkeyID_seq\"'::regclass)", column.default_function + assert column.serial? + end + end end class PrimaryKeyWithNoConnectionTest < ActiveRecord::TestCase diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md index 3b905b1d24..98954c7f73 100644 --- a/activesupport/CHANGELOG.md +++ b/activesupport/CHANGELOG.md @@ -1,9 +1,11 @@ -* Add Integer#positive? and Integer#negative? query methods in the vein of Fixnum#zero? - This makes it nicer to do things like bunch_of_numbers.select(&:positive?). +* Add `Integer#positive?` and `Integer#negative?` query methods + in the vein of `Fixnum#zero?`. + + This makes it nicer to do things like `bunch_of_numbers.select(&:positive?)`. *DHH* -* Encoding ActiveSupport::TimeWithZone to YAML now preserves the timezone information. +* Encoding `ActiveSupport::TimeWithZone` to YAML now preserves the timezone information. Fixes #9183. @@ -49,12 +51,12 @@ *Todd Bealmear* -* Fixed a problem where String#truncate_words would get stuck with a complex +* Fixed a problem where `String#truncate_words` would get stuck with a complex string. *Henrik Nygren* -* Fixed a roundtrip problem with AS::SafeBuffer where primitive-like strings +* Fixed a roundtrip problem with `AS::SafeBuffer` where primitive-like strings will be dumped as primitives: Before: @@ -101,7 +103,7 @@ *Ian Ker-Seymer* -* Duplicate frozen array when assigning it to a HashWithIndifferentAccess so +* Duplicate frozen array when assigning it to a `HashWithIndifferentAccess` so that it doesn't raise a `RuntimeError` when calling `map!` on it in `convert_value`. Fixes #18550. @@ -130,7 +132,7 @@ * Add `#on_weekend?`, `#next_weekday`, `#prev_weekday` methods to `Date`, `Time`, and `DateTime`. - `#on_weekend?` returns true if the receiving date/time falls on a Saturday + `#on_weekend?` returns `true` if the receiving date/time falls on a Saturday or Sunday. `#next_weekday` returns a new date/time representing the next day that does @@ -184,13 +186,13 @@ `Callbacks::CallbackChain.halt_and_display_warning_on_return_false`, will either not work at all or display a deprecation warning. -* Add Callbacks::CallbackChain.halt_and_display_warning_on_return_false +* Add `Callbacks::CallbackChain.halt_and_display_warning_on_return_false` Setting `Callbacks::CallbackChain.halt_and_display_warning_on_return_false` - to true will let an app support the deprecated way of halting callback + to `true` will let an app support the deprecated way of halting callback chains by returning `false`. - Setting the value to false will tell the app to ignore any `false` value + Setting the value to `false` will tell the app to ignore any `false` value returned by callbacks, and only halt the chain upon `throw(:abort)`. The value can also be set with the Rails configuration option @@ -203,7 +205,7 @@ *claudiob* -* Changes arguments and default value of CallbackChain's :terminator option +* Changes arguments and default value of CallbackChain's `:terminator` option Chains of callbacks defined without an explicit `:terminator` option will now be halted as soon as a `before_` callback throws `:abort`. diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb index 814fd288cf..e8ab3a7db5 100644 --- a/activesupport/lib/active_support/callbacks.rb +++ b/activesupport/lib/active_support/callbacks.rb @@ -638,7 +638,7 @@ module ActiveSupport # set_callback :save, :after, :after_meth, if: :condition # set_callback :save, :around, ->(r, block) { stuff; result = block.call; stuff } # - # The second arguments indicates whether the callback is to be run +:before+, + # The second argument indicates whether the callback is to be run +:before+, # +:after+, or +:around+ the event. If omitted, +:before+ is assumed. This # means the first example above can also be written as: # diff --git a/activesupport/lib/active_support/core_ext/integer.rb b/activesupport/lib/active_support/core_ext/integer.rb index f5b185b42b..a44a1b4c74 100644 --- a/activesupport/lib/active_support/core_ext/integer.rb +++ b/activesupport/lib/active_support/core_ext/integer.rb @@ -1,4 +1,3 @@ require 'active_support/core_ext/integer/multiple' require 'active_support/core_ext/integer/inflections' -require 'active_support/core_ext/integer/inquiry' require 'active_support/core_ext/integer/time' diff --git a/activesupport/lib/active_support/core_ext/numeric.rb b/activesupport/lib/active_support/core_ext/numeric.rb index a6bc0624be..bcdc3eace2 100644 --- a/activesupport/lib/active_support/core_ext/numeric.rb +++ b/activesupport/lib/active_support/core_ext/numeric.rb @@ -1,3 +1,4 @@ require 'active_support/core_ext/numeric/bytes' require 'active_support/core_ext/numeric/time' +require 'active_support/core_ext/numeric/inquiry' require 'active_support/core_ext/numeric/conversions' diff --git a/activesupport/lib/active_support/core_ext/integer/inquiry.rb b/activesupport/lib/active_support/core_ext/numeric/inquiry.rb index 17a04d4d63..7e7ac1b0b2 100644 --- a/activesupport/lib/active_support/core_ext/integer/inquiry.rb +++ b/activesupport/lib/active_support/core_ext/numeric/inquiry.rb @@ -1,4 +1,5 @@ -class Integer +unless 1.respond_to?(:positive?) # TODO: Remove this file when we drop support to ruby < 2.3 +class Numeric # Returns true if the number is positive. # # 1.positive? # => true @@ -17,3 +18,9 @@ class Integer self < 0 end end + +class Complex + undef :positive? + undef :negative? +end +end diff --git a/activesupport/lib/active_support/inflector/methods.rb b/activesupport/lib/active_support/inflector/methods.rb index a08c655d69..bde70d772d 100644 --- a/activesupport/lib/active_support/inflector/methods.rb +++ b/activesupport/lib/active_support/inflector/methods.rb @@ -231,8 +231,8 @@ module ActiveSupport # Tries to find a constant with the name specified in the argument string. # - # 'Module'.constantize # => Module - # 'Test::Unit'.constantize # => Test::Unit + # 'Module'.constantize # => Module + # 'Foo::Bar'.constantize # => Foo::Bar # # The name is assumed to be the one of a top-level constant, no matter # whether it starts with "::" or not. No lexical context is taken into @@ -280,8 +280,8 @@ module ActiveSupport # Tries to find a constant with the name specified in the argument string. # - # safe_constantize('Module') # => Module - # safe_constantize('Test::Unit') # => Test::Unit + # safe_constantize('Module') # => Module + # safe_constantize('Foo::Bar') # => Foo::Bar # # The name is assumed to be the one of a top-level constant, no matter # whether it starts with "::" or not. No lexical context is taken into diff --git a/activesupport/test/core_ext/integer_ext_test.rb b/activesupport/test/core_ext/integer_ext_test.rb index 6eeadb2ace..41736fb672 100644 --- a/activesupport/test/core_ext/integer_ext_test.rb +++ b/activesupport/test/core_ext/integer_ext_test.rb @@ -27,16 +27,4 @@ class IntegerExtTest < ActiveSupport::TestCase assert_equal 'st', 1.ordinal assert_equal 'th', 8.ordinal end - - def test_positive - assert 1.positive? - assert_not 0.positive? - assert_not -1.positive? - end - - def test_negative - assert -1.negative? - assert_not 0.negative? - assert_not 1.negative? - end end diff --git a/activesupport/test/core_ext/numeric_ext_test.rb b/activesupport/test/core_ext/numeric_ext_test.rb index b82448458d..2d8796179e 100644 --- a/activesupport/test/core_ext/numeric_ext_test.rb +++ b/activesupport/test/core_ext/numeric_ext_test.rb @@ -389,4 +389,88 @@ class NumericExtFormattingTest < ActiveSupport::TestCase def test_in_milliseconds assert_equal 10_000, 10.seconds.in_milliseconds end + + # TODO: Remove positive and negative tests when we drop support to ruby < 2.3 + b = 2**64 + b *= b until Bignum === b + + T_ZERO = b.coerce(0).first + T_ONE = b.coerce(1).first + T_MONE = b.coerce(-1).first + + def test_positive + assert_predicate(1, :positive?) + assert_not_predicate(0, :positive?) + assert_not_predicate(-1, :positive?) + assert_predicate(+1.0, :positive?) + assert_not_predicate(+0.0, :positive?) + assert_not_predicate(-0.0, :positive?) + assert_not_predicate(-1.0, :positive?) + assert_predicate(+(0.0.next_float), :positive?) + assert_not_predicate(-(0.0.next_float), :positive?) + assert_predicate(Float::INFINITY, :positive?) + assert_not_predicate(-Float::INFINITY, :positive?) + assert_not_predicate(Float::NAN, :positive?) + + a = Class.new(Numeric) do + def >(x); true; end + end.new + assert_predicate(a, :positive?) + + a = Class.new(Numeric) do + def >(x); false; end + end.new + assert_not_predicate(a, :positive?) + + assert_predicate(1/2r, :positive?) + assert_not_predicate(-1/2r, :positive?) + + assert_predicate(T_ONE, :positive?) + assert_not_predicate(T_MONE, :positive?) + assert_not_predicate(T_ZERO, :positive?) + + e = assert_raises(NoMethodError) do + Complex(1).positive? + end + + assert_match(/positive\?/, e.message) + end + + def test_negative + assert_predicate(-1, :negative?) + assert_not_predicate(0, :negative?) + assert_not_predicate(1, :negative?) + assert_predicate(-1.0, :negative?) + assert_not_predicate(-0.0, :negative?) + assert_not_predicate(+0.0, :negative?) + assert_not_predicate(+1.0, :negative?) + assert_predicate(-(0.0.next_float), :negative?) + assert_not_predicate(+(0.0.next_float), :negative?) + assert_predicate(-Float::INFINITY, :negative?) + assert_not_predicate(Float::INFINITY, :negative?) + assert_not_predicate(Float::NAN, :negative?) + + a = Class.new(Numeric) do + def <(x); true; end + end.new + assert_predicate(a, :negative?) + + a = Class.new(Numeric) do + def <(x); false; end + end.new + assert_not_predicate(a, :negative?) + + assert_predicate(-1/2r, :negative?) + assert_not_predicate(1/2r, :negative?) + + assert_not_predicate(T_ONE, :negative?) + assert_predicate(T_MONE, :negative?) + assert_not_predicate(T_ZERO, :negative?) + + e = assert_raises(NoMethodError) do + Complex(1).negative? + end + + assert_match(/negative\?/, e.message) + end end diff --git a/guides/source/active_record_validations.md b/guides/source/active_record_validations.md index 343b761e93..8d41987958 100644 --- a/guides/source/active_record_validations.md +++ b/guides/source/active_record_validations.md @@ -47,7 +47,7 @@ built-in helpers for common needs, and allows you to create your own validation methods as well. There are several other ways to validate data before it is saved into your -database, including native database constraints, client-side validations, +database, including native database constraints, client-side validations and controller-level validations. Here's a summary of the pros and cons: * Database constraints and/or stored procedures make the validation mechanisms @@ -122,7 +122,7 @@ database only if the object is valid: * `update!` The bang versions (e.g. `save!`) raise an exception if the record is invalid. -The non-bang versions don't, `save` and `update` return `false`, +The non-bang versions don't: `save` and `update` return `false`, and `create` just returns the object. ### Skipping Validations @@ -143,7 +143,7 @@ database regardless of its validity. They should be used with caution. * `update_counters` Note that `save` also has the ability to skip validations if passed `validate: -false` as argument. This technique should be used with caution. +false` as an argument. This technique should be used with caution. * `save(validate: false)` @@ -272,7 +272,7 @@ available helpers. This method validates that a checkbox on the user interface was checked when a form was submitted. This is typically used when the user needs to agree to your -application's terms of service, confirm reading some text, or any similar +application's terms of service, confirm that some text is read, or any similar concept. This validation is very specific to web applications and this 'acceptance' does not need to be recorded anywhere in your database (if you don't have a field for it, the helper will just create a virtual attribute). @@ -338,7 +338,7 @@ In your view template you could use something like This check is performed only if `email_confirmation` is not `nil`. To require confirmation, make sure to add a presence check for the confirmation attribute -(we'll take a look at `presence` later on this guide): +(we'll take a look at `presence` later on in this guide): ```ruby class Person < ActiveRecord::Base @@ -499,9 +499,9 @@ constraints to acceptable values: default error message for this option is _"must be equal to %{count}"_. * `:less_than` - Specifies the value must be less than the supplied value. The default error message for this option is _"must be less than %{count}"_. -* `:less_than_or_equal_to` - Specifies the value must be less than or equal the - supplied value. The default error message for this option is _"must be less - than or equal to %{count}"_. +* `:less_than_or_equal_to` - Specifies the value must be less than or equal to + the supplied value. The default error message for this option is _"must be + less than or equal to %{count}"_. * `:odd` - Specifies the value must be an odd number if set to true. The default error message for this option is _"must be odd"_. * `:even` - Specifies the value must be an even number if set to true. The @@ -813,7 +813,7 @@ end Person.new.valid? # => ActiveModel::StrictValidationFailed: Name can't be blank ``` -There is also an ability to pass custom exception to `:strict` option. +There is also the ability to pass a custom exception to the `:strict` option. ```ruby class Person < ActiveRecord::Base @@ -877,7 +877,7 @@ end ### Grouping Conditional validations -Sometimes it is useful to have multiple validations use one condition, it can +Sometimes it is useful to have multiple validations use one condition. It can be easily achieved using `with_options`. ```ruby @@ -889,8 +889,8 @@ class User < ActiveRecord::Base end ``` -All validations inside of `with_options` block will have automatically passed -the condition `if: :is_admin?` +All validations inside of the `with_options` block will have automatically +passed the condition `if: :is_admin?` ### Combining Validation Conditions diff --git a/guides/source/configuring.md b/guides/source/configuring.md index 342d39a528..9e2588ecb3 100644 --- a/guides/source/configuring.md +++ b/guides/source/configuring.md @@ -187,13 +187,14 @@ The full set of methods that can be used in this block are as follows: * `helper` defines whether or not to generate helpers. Defaults to `true`. * `integration_tool` defines which integration tool to use. Defaults to `nil`. * `javascripts` turns on the hook for JavaScript files in generators. Used in Rails for when the `scaffold` generator is run. Defaults to `true`. -* `javascript_engine` configures the engine to be used (for eg. coffee) when generating assets. Defaults to `nil`. +* `javascript_engine` configures the engine to be used (for eg. coffee) when generating assets. Defaults to `:js`. * `orm` defines which orm to use. Defaults to `false` and will use Active Record by default. * `resource_controller` defines which generator to use for generating a controller when using `rails generate resource`. Defaults to `:controller`. +* `resource_route` defines whether inject resource route definition in routes or not. Defaults to `true`. * `scaffold_controller` different from `resource_controller`, defines which generator to use for generating a _scaffolded_ controller when using `rails generate scaffold`. Defaults to `:scaffold_controller`. * `stylesheets` turns on the hook for stylesheets in generators. Used in Rails for when the `scaffold` generator is run, but this hook can be used in other generates as well. Defaults to `true`. * `stylesheet_engine` configures the stylesheet engine (for eg. sass) to be used when generating assets. Defaults to `:css`. -* `test_framework` defines which test framework to use. Defaults to `false` and will use Test::Unit by default. +* `test_framework` defines which test framework to use. Defaults to `false` and will use Minitest by default. * `template_engine` defines which template engine to use, such as ERB or Haml. Defaults to `:erb`. ### Configuring Middleware diff --git a/guides/source/security.md b/guides/source/security.md index a464b83838..05d87bba14 100644 --- a/guides/source/security.md +++ b/guides/source/security.md @@ -1033,4 +1033,5 @@ The security landscape shifts and it is important to keep up to date, because mi * Subscribe to the Rails security [mailing list](http://groups.google.com/group/rubyonrails-security) * [Keep up to date on the other application layers](http://secunia.com/) (they have a weekly newsletter, too) -* A [good security blog](http://ha.ckers.org/blog/) including the [Cross-Site scripting Cheat Sheet](http://ha.ckers.org/xss.html) +* A [good security blog](https://www.owasp.org) including the [Cross-Site scripting Cheat Sheet](https://www.owasp.org/index.php/DOM_based_XSS_Prevention_Cheat_Sheet) + |