diff options
57 files changed, 338 insertions, 544 deletions
@@ -48,7 +48,7 @@ group :job do gem 'resque', require: false gem 'resque-scheduler', require: false gem 'sidekiq', require: false - gem 'sucker_punch', '< 2.0', require: false + gem 'sucker_punch', require: false gem 'delayed_job', require: false gem 'queue_classic', github: "QueueClassic/queue_classic", branch: 'master', require: false, platforms: :ruby gem 'sneakers', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 61b90d305c..f02a85f70d 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -29,61 +29,62 @@ GIT PATH remote: . specs: - actioncable (5.0.0.beta1) - actionpack (= 5.0.0.beta1) + actioncable (5.0.0.beta1.1) + actionpack (= 5.0.0.beta1.1) coffee-rails (~> 4.1.0) - nio4r (~> 1.2) + eventmachine (~> 1.0) + faye-websocket (~> 0.10.0) websocket-driver (~> 0.6.1) - actionmailer (5.0.0.beta1) - actionpack (= 5.0.0.beta1) - actionview (= 5.0.0.beta1) - activejob (= 5.0.0.beta1) + actionmailer (5.0.0.beta1.1) + actionpack (= 5.0.0.beta1.1) + actionview (= 5.0.0.beta1.1) + activejob (= 5.0.0.beta1.1) mail (~> 2.5, >= 2.5.4) rails-dom-testing (~> 1.0, >= 1.0.5) - actionpack (5.0.0.beta1) - actionview (= 5.0.0.beta1) - activesupport (= 5.0.0.beta1) + actionpack (5.0.0.beta1.1) + actionview (= 5.0.0.beta1.1) + activesupport (= 5.0.0.beta1.1) rack (~> 2.x) rack-test (~> 0.6.3) rails-dom-testing (~> 1.0, >= 1.0.5) rails-html-sanitizer (~> 1.0, >= 1.0.2) - actionview (5.0.0.beta1) - activesupport (= 5.0.0.beta1) + actionview (5.0.0.beta1.1) + activesupport (= 5.0.0.beta1.1) builder (~> 3.1) erubis (~> 2.7.0) rails-dom-testing (~> 1.0, >= 1.0.5) rails-html-sanitizer (~> 1.0, >= 1.0.2) - activejob (5.0.0.beta1) - activesupport (= 5.0.0.beta1) + activejob (5.0.0.beta1.1) + activesupport (= 5.0.0.beta1.1) globalid (>= 0.3.6) - activemodel (5.0.0.beta1) - activesupport (= 5.0.0.beta1) - activerecord (5.0.0.beta1) - activemodel (= 5.0.0.beta1) - activesupport (= 5.0.0.beta1) + activemodel (5.0.0.beta1.1) + activesupport (= 5.0.0.beta1.1) + activerecord (5.0.0.beta1.1) + activemodel (= 5.0.0.beta1.1) + activesupport (= 5.0.0.beta1.1) arel (~> 7.0) - activesupport (5.0.0.beta1) + activesupport (5.0.0.beta1.1) concurrent-ruby (~> 1.0) i18n (~> 0.7) json (~> 1.7, >= 1.7.7) method_source minitest (~> 5.1) tzinfo (~> 1.1) - rails (5.0.0.beta1) - actioncable (= 5.0.0.beta1) - actionmailer (= 5.0.0.beta1) - actionpack (= 5.0.0.beta1) - actionview (= 5.0.0.beta1) - activejob (= 5.0.0.beta1) - activemodel (= 5.0.0.beta1) - activerecord (= 5.0.0.beta1) - activesupport (= 5.0.0.beta1) + rails (5.0.0.beta1.1) + actioncable (= 5.0.0.beta1.1) + actionmailer (= 5.0.0.beta1.1) + actionpack (= 5.0.0.beta1.1) + actionview (= 5.0.0.beta1.1) + activejob (= 5.0.0.beta1.1) + activemodel (= 5.0.0.beta1.1) + activerecord (= 5.0.0.beta1.1) + activesupport (= 5.0.0.beta1.1) bundler (>= 1.3.0, < 2.0) - railties (= 5.0.0.beta1) + railties (= 5.0.0.beta1.1) sprockets-rails (>= 2.0.0) - railties (5.0.0.beta1) - actionpack (= 5.0.0.beta1) - activesupport (= 5.0.0.beta1) + railties (5.0.0.beta1.1) + actionpack (= 5.0.0.beta1.1) + activesupport (= 5.0.0.beta1.1) method_source rake (>= 0.8.7) thor (>= 0.18.1, < 2.0) @@ -105,23 +106,6 @@ GEM bunny (2.2.1) amq-protocol (>= 2.0.0) byebug (8.2.1) - celluloid (0.17.2) - celluloid-essentials - celluloid-extras - celluloid-fsm - celluloid-pool - celluloid-supervision - timers (>= 4.1.1) - celluloid-essentials (0.20.5) - timers (>= 4.1.1) - celluloid-extras (0.20.5) - timers (>= 4.1.1) - celluloid-fsm (0.20.5) - timers (>= 4.1.1) - celluloid-pool (0.20.5) - timers (>= 4.1.1) - celluloid-supervision (0.20.5) - timers (>= 4.1.1) coffee-rails (4.1.0) coffee-script (>= 2.2.0) railties (>= 4.0.0, < 5.0) @@ -144,14 +128,15 @@ GEM erubis (2.7.0) eventmachine (1.0.9.1) execjs (2.6.0) + faye-websocket (0.10.2) + eventmachine (>= 0.12.0) + websocket-driver (>= 0.5.1) ffi (1.9.10) ffi (1.9.10-x64-mingw32) ffi (1.9.10-x86-mingw32) globalid (0.3.6) activesupport (>= 4.1.0) hiredis (0.5.2) - hitimes (1.2.3) - hitimes (1.2.3-x86-mingw32) i18n (0.7.0) jquery-rails (4.0.5) rails-dom-testing (~> 1.0) @@ -181,7 +166,6 @@ GEM mysql2 (0.4.2) mysql2 (0.4.2-x64-mingw32) mysql2 (0.4.2-x86-mingw32) - nio4r (1.2.0) nokogiri (1.6.7.1) mini_portile2 (~> 2.0.0.rc2) nokogiri (1.6.7.1-x64-mingw32) @@ -260,13 +244,11 @@ GEM sqlite3 (1.3.11-x64-mingw32) sqlite3 (1.3.11-x86-mingw32) stackprof (0.2.7) - sucker_punch (1.6.0) - celluloid (~> 0.17.2) + sucker_punch (2.0.0) + concurrent-ruby (~> 1.0.0) thor (0.19.1) thread (0.1.7) thread_safe (0.3.5) - timers (4.1.1) - hitimes turbolinks (2.5.3) coffee-rails tzinfo (1.2.2) @@ -334,7 +316,7 @@ DEPENDENCIES sneakers sqlite3 (~> 1.3.6) stackprof - sucker_punch (< 2.0) + sucker_punch turbolinks tzinfo-data uglifier (>= 1.3.0) diff --git a/actioncable/README.md b/actioncable/README.md index 63e328321b..cad71ddf94 100644 --- a/actioncable/README.md +++ b/actioncable/README.md @@ -443,11 +443,10 @@ The Ruby side of things is built on top of [faye-websocket](https://github.com/f ## Deployment -Action Cable is powered by a combination of EventMachine and threads. The -framework plumbing needed for connection handling is handled in the -EventMachine loop, but the actual channel, user-specified, work is handled -in a normal Ruby thread. This means you can use all your regular Rails models -with no problem, as long as you haven't committed any thread-safety sins. +Action Cable is powered by a combination of websockets and threads. All of the +connection management is handled internally by utilizing Ruby’s native thread +support, which means you can use all your regular Rails models with no problems +as long as you haven’t committed any thread-safety sins. But this also means that Action Cable needs to run in its own server process. So you'll have one set of server processes for your normal web work, and another diff --git a/actioncable/actioncable.gemspec b/actioncable/actioncable.gemspec index 14f968f1ef..a36acc8f6f 100644 --- a/actioncable/actioncable.gemspec +++ b/actioncable/actioncable.gemspec @@ -21,7 +21,8 @@ Gem::Specification.new do |s| s.add_dependency 'actionpack', version s.add_dependency 'coffee-rails', '~> 4.1.0' - s.add_dependency 'nio4r', '~> 1.2' + s.add_dependency 'eventmachine', '~> 1.0' + s.add_dependency 'faye-websocket', '~> 0.10.0' s.add_dependency 'websocket-driver', '~> 0.6.1' s.add_development_dependency 'em-hiredis', '~> 0.3.0' diff --git a/actioncable/lib/action_cable/channel/periodic_timers.rb b/actioncable/lib/action_cable/channel/periodic_timers.rb index 56597d02d7..7f0fb37afc 100644 --- a/actioncable/lib/action_cable/channel/periodic_timers.rb +++ b/actioncable/lib/action_cable/channel/periodic_timers.rb @@ -27,14 +27,14 @@ module ActionCable def start_periodic_timers self.class.periodic_timers.each do |callback, options| - active_periodic_timers << Concurrent::TimerTask.new(execution_interval: options[:every]) do + active_periodic_timers << EventMachine::PeriodicTimer.new(options[:every]) do connection.worker_pool.async_run_periodic_timer(self, callback) end end end def stop_periodic_timers - active_periodic_timers.each { |timer| timer.shutdown } + active_periodic_timers.each { |timer| timer.cancel } end end end diff --git a/actioncable/lib/action_cable/channel/streams.rb b/actioncable/lib/action_cable/channel/streams.rb index a26373e387..e2876ef6fa 100644 --- a/actioncable/lib/action_cable/channel/streams.rb +++ b/actioncable/lib/action_cable/channel/streams.rb @@ -75,7 +75,7 @@ module ActionCable callback ||= default_stream_callback(broadcasting) streams << [ broadcasting, callback ] - Concurrent.global_io_executor.post do + EM.next_tick do pubsub.subscribe(broadcasting, callback, lambda do transmit_subscription_confirmation logger.info "#{self.class.name} is streaming from #{broadcasting}" diff --git a/actioncable/lib/action_cable/connection.rb b/actioncable/lib/action_cable/connection.rb index 902efb07e2..b672e00682 100644 --- a/actioncable/lib/action_cable/connection.rb +++ b/actioncable/lib/action_cable/connection.rb @@ -5,15 +5,12 @@ module ActionCable eager_autoload do autoload :Authorization autoload :Base - autoload :ClientSocket autoload :Identification autoload :InternalChannel autoload :MessageBuffer - autoload :Stream - autoload :StreamEventLoop + autoload :WebSocket autoload :Subscriptions autoload :TaggedLoggerProxy - autoload :WebSocket end end end diff --git a/actioncable/lib/action_cable/connection/base.rb b/actioncable/lib/action_cable/connection/base.rb index 0016d1a1a4..bb8850aaa0 100644 --- a/actioncable/lib/action_cable/connection/base.rb +++ b/actioncable/lib/action_cable/connection/base.rb @@ -49,14 +49,14 @@ module ActionCable include Authorization attr_reader :server, :env, :subscriptions, :logger - delegate :stream_event_loop, :worker_pool, :pubsub, to: :server + delegate :worker_pool, :pubsub, to: :server def initialize(server, env) @server, @env = server, env @logger = new_tagged_logger - @websocket = ActionCable::Connection::WebSocket.new(env, self, stream_event_loop) + @websocket = ActionCable::Connection::WebSocket.new(env) @subscriptions = ActionCable::Connection::Subscriptions.new(self) @message_buffer = ActionCable::Connection::MessageBuffer.new(self) @@ -70,6 +70,10 @@ module ActionCable logger.info started_request_message if websocket.possible? && allow_request_origin? + websocket.on(:open) { |event| send_async :on_open } + websocket.on(:message) { |event| on_message event.data } + websocket.on(:close) { |event| send_async :on_close } + respond_to_successful_request else respond_to_invalid_request @@ -117,22 +121,6 @@ module ActionCable transmit ActiveSupport::JSON.encode(identifier: ActionCable::INTERNAL[:identifiers][:ping], message: Time.now.to_i) end - def on_open # :nodoc: - send_async :handle_open - end - - def on_message(message) # :nodoc: - message_buffer.append message - end - - def on_error(message) # :nodoc: - # ignore - end - - def on_close # :nodoc: - send_async :handle_close - end - protected # The request that initiated the WebSocket connection is available here. This gives access to the environment, cookies, etc. def request @@ -151,7 +139,7 @@ module ActionCable attr_reader :message_buffer private - def handle_open + def on_open connect if respond_to?(:connect) subscribe_to_internal_channel beat @@ -162,7 +150,11 @@ module ActionCable respond_to_invalid_request end - def handle_close + def on_message(message) + message_buffer.append message + end + + def on_close logger.info finished_request_message server.remove_connection(self) diff --git a/actioncable/lib/action_cable/connection/client_socket.rb b/actioncable/lib/action_cable/connection/client_socket.rb deleted file mode 100644 index 62dd753646..0000000000 --- a/actioncable/lib/action_cable/connection/client_socket.rb +++ /dev/null @@ -1,152 +0,0 @@ -require 'websocket/driver' - -module ActionCable - module Connection - #-- - # This class is heavily based on faye-websocket-ruby - # - # Copyright (c) 2010-2015 James Coglan - class ClientSocket # :nodoc: - def self.determine_url(env) - scheme = secure_request?(env) ? 'wss:' : 'ws:' - "#{ scheme }//#{ env['HTTP_HOST'] }#{ env['REQUEST_URI'] }" - end - - def self.secure_request?(env) - return true if env['HTTPS'] == 'on' - return true if env['HTTP_X_FORWARDED_SSL'] == 'on' - return true if env['HTTP_X_FORWARDED_SCHEME'] == 'https' - return true if env['HTTP_X_FORWARDED_PROTO'] == 'https' - return true if env['rack.url_scheme'] == 'https' - - return false - end - - CONNECTING = 0 - OPEN = 1 - CLOSING = 2 - CLOSED = 3 - - attr_reader :env, :url - - def initialize(env, event_target, stream_event_loop) - @env = env - @event_target = event_target - @stream_event_loop = stream_event_loop - - @url = ClientSocket.determine_url(@env) - - @driver = @driver_started = nil - - @ready_state = CONNECTING - - # The driver calls +env+, +url+, and +write+ - @driver = ::WebSocket::Driver.rack(self) - - @driver.on(:open) { |e| open } - @driver.on(:message) { |e| receive_message(e.data) } - @driver.on(:close) { |e| begin_close(e.reason, e.code) } - @driver.on(:error) { |e| emit_error(e.message) } - - @stream = ActionCable::Connection::Stream.new(@stream_event_loop, self) - - if callback = @env['async.callback'] - callback.call([101, {}, @stream]) - end - end - - def start_driver - return if @driver.nil? || @driver_started - @driver_started = true - @driver.start - end - - def rack_response - start_driver - [ -1, {}, [] ] - end - - def write(data) - @stream.write(data) - end - - def transmit(message) - return false if @ready_state > OPEN - case message - when Numeric then @driver.text(message.to_s) - when String then @driver.text(message) - when Array then @driver.binary(message) - else false - end - end - - def close(code = nil, reason = nil) - code ||= 1000 - reason ||= '' - - unless code == 1000 or (code >= 3000 and code <= 4999) - raise ArgumentError, "Failed to execute 'close' on WebSocket: " + - "The code must be either 1000, or between 3000 and 4999. " + - "#{code} is neither." - end - - @ready_state = CLOSING unless @ready_state == CLOSED - @driver.close(reason, code) - end - - def parse(data) - @driver.parse(data) - end - - def client_gone - finalize_close - end - - def alive? - @ready_state == OPEN - end - - private - def open - return unless @ready_state == CONNECTING - @ready_state = OPEN - - @event_target.on_open - end - - def receive_message(data) - return unless @ready_state == OPEN - - @event_target.on_message(data) - end - - def emit_error(message) - return if @ready_state >= CLOSING - - @event_target.on_error(message) - end - - def begin_close(reason, code) - return if @ready_state == CLOSED - @ready_state = CLOSING - @close_params = [reason, code] - - if @stream - @stream.shutdown - else - finalize_close - end - end - - def finalize_close - return if @ready_state == CLOSED - @ready_state = CLOSED - - reason = @close_params ? @close_params[0] : '' - code = @close_params ? @close_params[1] : 1006 - - @event_target.on_close(code, reason) - end - end - end -end diff --git a/actioncable/lib/action_cable/connection/internal_channel.rb b/actioncable/lib/action_cable/connection/internal_channel.rb index 27826792b3..54ed7672d2 100644 --- a/actioncable/lib/action_cable/connection/internal_channel.rb +++ b/actioncable/lib/action_cable/connection/internal_channel.rb @@ -15,14 +15,14 @@ module ActionCable @_internal_subscriptions ||= [] @_internal_subscriptions << [ internal_channel, callback ] - Concurrent.global_io_executor.post { pubsub.subscribe(internal_channel, callback) } + EM.next_tick { pubsub.subscribe(internal_channel, callback) } logger.info "Registered connection (#{connection_identifier})" end end def unsubscribe_from_internal_channel if @_internal_subscriptions.present? - @_internal_subscriptions.each { |channel, callback| Concurrent.global_io_executor.post { pubsub.unsubscribe(channel, callback) } } + @_internal_subscriptions.each { |channel, callback| EM.next_tick { pubsub.unsubscribe(channel, callback) } } end end diff --git a/actioncable/lib/action_cable/connection/stream.rb b/actioncable/lib/action_cable/connection/stream.rb deleted file mode 100644 index ace250cd16..0000000000 --- a/actioncable/lib/action_cable/connection/stream.rb +++ /dev/null @@ -1,59 +0,0 @@ -module ActionCable - module Connection - #-- - # This class is heavily based on faye-websocket-ruby - # - # Copyright (c) 2010-2015 James Coglan - class Stream - def initialize(event_loop, socket) - @event_loop = event_loop - @socket_object = socket - @stream_send = socket.env['stream.send'] - - @rack_hijack_io = nil - - hijack_rack_socket - end - - def each(&callback) - @stream_send ||= callback - end - - def close - shutdown - @socket_object.client_gone - end - - def shutdown - clean_rack_hijack - end - - def write(data) - return @rack_hijack_io.write(data) if @rack_hijack_io - return @stream_send.call(data) if @stream_send - rescue EOFError - @socket_object.client_gone - end - - def receive(data) - @socket_object.parse(data) - end - - private - def hijack_rack_socket - return unless @socket_object.env['rack.hijack'] - - @socket_object.env['rack.hijack'].call - @rack_hijack_io = @socket_object.env['rack.hijack_io'] - - @event_loop.attach(@rack_hijack_io, self) - end - - def clean_rack_hijack - return unless @rack_hijack_io - @event_loop.detach(@rack_hijack_io, self) - @rack_hijack_io = nil - end - end - end -end diff --git a/actioncable/lib/action_cable/connection/stream_event_loop.rb b/actioncable/lib/action_cable/connection/stream_event_loop.rb deleted file mode 100644 index f773814973..0000000000 --- a/actioncable/lib/action_cable/connection/stream_event_loop.rb +++ /dev/null @@ -1,68 +0,0 @@ -require 'nio' - -module ActionCable - module Connection - class StreamEventLoop - def initialize - @nio = NIO::Selector.new - @map = {} - @stopping = false - @todo = Queue.new - - Thread.new do - Thread.current.abort_on_exception = true - run - end - end - - def attach(io, stream) - @todo << lambda do - @map[io] = stream - @nio.register(io, :r) - end - @nio.wakeup - end - - def detach(io, stream) - @todo << lambda do - @nio.deregister(io) - @map.delete io - end - @nio.wakeup - end - - def stop - @stopping = true - @nio.wakeup - end - - def run - loop do - if @stopping - @nio.close - break - end - - until @todo.empty? - @todo.pop(true).call - end - - if monitors = @nio.select - monitors.each do |monitor| - io = monitor.io - stream = @map[io] - - begin - stream.receive io.read_nonblock(4096) - rescue IO::WaitReadable - next - rescue EOFError - stream.close - end - end - end - end - end - end - end -end diff --git a/actioncable/lib/action_cable/connection/web_socket.rb b/actioncable/lib/action_cable/connection/web_socket.rb index 5e89fb9b72..670d5690ae 100644 --- a/actioncable/lib/action_cable/connection/web_socket.rb +++ b/actioncable/lib/action_cable/connection/web_socket.rb @@ -1,11 +1,13 @@ -require 'websocket/driver' +require 'faye/websocket' module ActionCable module Connection - # Wrap the real socket to minimize the externally-presented API + # Decorate the Faye::WebSocket with helpers we need. class WebSocket - def initialize(env, event_target, stream_event_loop) - @websocket = ::WebSocket::Driver.websocket?(env) ? ClientSocket.new(env, event_target, stream_event_loop) : nil + delegate :rack_response, :close, :on, to: :websocket + + def initialize(env) + @websocket = Faye::WebSocket.websocket?(env) ? Faye::WebSocket.new(env) : nil end def possible? @@ -13,19 +15,11 @@ module ActionCable end def alive? - websocket && websocket.alive? + websocket && websocket.ready_state == Faye::WebSocket::API::OPEN end def transmit(data) - websocket.transmit data - end - - def close - websocket.close - end - - def rack_response - websocket.rack_response + websocket.send data end protected diff --git a/actioncable/lib/action_cable/process/logging.rb b/actioncable/lib/action_cable/process/logging.rb new file mode 100644 index 0000000000..dce637b3ca --- /dev/null +++ b/actioncable/lib/action_cable/process/logging.rb @@ -0,0 +1,7 @@ +require 'action_cable/server' +require 'eventmachine' + +EM.error_handler do |e| + puts "Error raised inside the event loop: #{e.message}" + puts e.backtrace.join("\n") +end diff --git a/actioncable/lib/action_cable/server.rb b/actioncable/lib/action_cable/server.rb index bd6a3826a3..a2a89d5f1e 100644 --- a/actioncable/lib/action_cable/server.rb +++ b/actioncable/lib/action_cable/server.rb @@ -1,3 +1,7 @@ +require 'eventmachine' +EventMachine.epoll if EventMachine.epoll? +EventMachine.kqueue if EventMachine.kqueue? + module ActionCable module Server extend ActiveSupport::Autoload diff --git a/actioncable/lib/action_cable/server/base.rb b/actioncable/lib/action_cable/server/base.rb index b00abd208c..3385a4c9f3 100644 --- a/actioncable/lib/action_cable/server/base.rb +++ b/actioncable/lib/action_cable/server/base.rb @@ -32,10 +32,6 @@ module ActionCable @remote_connections ||= RemoteConnections.new(self) end - def stream_event_loop - @stream_event_loop ||= ActionCable::Connection::StreamEventLoop.new - end - # The thread worker pool for handling all the connection work on this server. Default size is set by config.worker_pool_size. def worker_pool @worker_pool ||= ActionCable::Server::Worker.new(max_size: config.worker_pool_size) diff --git a/actioncable/lib/action_cable/server/connections.rb b/actioncable/lib/action_cable/server/connections.rb index 8671dd5ebd..47dcea8c20 100644 --- a/actioncable/lib/action_cable/server/connections.rb +++ b/actioncable/lib/action_cable/server/connections.rb @@ -22,9 +22,11 @@ module ActionCable # then can't rely on being able to receive and send to it. So there's a 3 second heartbeat running on all connections. If the beat fails, we automatically # disconnect. def setup_heartbeat_timer - @heartbeat_timer ||= Concurrent::TimerTask.new(execution_interval: BEAT_INTERVAL) do - Concurrent.global_io_executor.post { connections.map(&:beat) } - end.tap(&:execute) + EM.next_tick do + @heartbeat_timer ||= EventMachine.add_periodic_timer(BEAT_INTERVAL) do + EM.next_tick { connections.map(&:beat) } + end + end end def open_connections_statistics diff --git a/actioncable/lib/action_cable/subscription_adapter/async.rb b/actioncable/lib/action_cable/subscription_adapter/async.rb index c88b03947a..85d4892e4c 100644 --- a/actioncable/lib/action_cable/subscription_adapter/async.rb +++ b/actioncable/lib/action_cable/subscription_adapter/async.rb @@ -10,11 +10,11 @@ module ActionCable class AsyncSubscriberMap < SubscriberMap def add_subscriber(*) - Concurrent.global_io_executor.post { super } + ::EM.next_tick { super } end def invoke_callback(*) - Concurrent.global_io_executor.post { super } + ::EM.next_tick { super } end end end diff --git a/actioncable/lib/action_cable/subscription_adapter/postgresql.rb b/actioncable/lib/action_cable/subscription_adapter/postgresql.rb index 3ce1bbed68..78f8aeb599 100644 --- a/actioncable/lib/action_cable/subscription_adapter/postgresql.rb +++ b/actioncable/lib/action_cable/subscription_adapter/postgresql.rb @@ -63,7 +63,7 @@ module ActionCable case action when :listen pg_conn.exec("LISTEN #{pg_conn.escape_identifier channel}") - Concurrent.global_io_executor << callback if callback + ::EM.next_tick(&callback) if callback when :unlisten pg_conn.exec("UNLISTEN #{pg_conn.escape_identifier channel}") when :shutdown @@ -93,7 +93,7 @@ module ActionCable end def invoke_callback(*) - Concurrent.global_io_executor.post { super } + ::EM.next_tick { super } end end end diff --git a/actioncable/lib/action_cable/subscription_adapter/redis.rb b/actioncable/lib/action_cable/subscription_adapter/redis.rb index a035e3988d..3b86354621 100644 --- a/actioncable/lib/action_cable/subscription_adapter/redis.rb +++ b/actioncable/lib/action_cable/subscription_adapter/redis.rb @@ -1,18 +1,11 @@ -require 'thread' - gem 'em-hiredis', '~> 0.3.0' gem 'redis', '~> 3.0' require 'em-hiredis' require 'redis' -EventMachine.epoll if EventMachine.epoll? -EventMachine.kqueue if EventMachine.kqueue? - module ActionCable module SubscriptionAdapter class Redis < Base # :nodoc: - @@mutex = Mutex.new - def broadcast(channel, payload) redis_connection_for_broadcasts.publish(channel, payload) end @@ -34,7 +27,6 @@ module ActionCable private def redis_connection_for_subscriptions - ensure_reactor_running @redis_connection_for_subscriptions ||= EM::Hiredis.connect(@server.config.cable[:url]).tap do |redis| redis.on(:reconnect_failed) do @logger.info "[ActionCable] Redis reconnect failed." @@ -45,14 +37,6 @@ module ActionCable def redis_connection_for_broadcasts @redis_connection_for_broadcasts ||= ::Redis.new(@server.config.cable) end - - def ensure_reactor_running - return if EventMachine.reactor_running? - @@mutex.synchronize do - Thread.new { EventMachine.run } unless EventMachine.reactor_running? - Thread.pass until EventMachine.reactor_running? - end - end end end end diff --git a/actioncable/test/channel/periodic_timers_test.rb b/actioncable/test/channel/periodic_timers_test.rb index 64f0247cd6..1590a12f09 100644 --- a/actioncable/test/channel/periodic_timers_test.rb +++ b/actioncable/test/channel/periodic_timers_test.rb @@ -31,7 +31,7 @@ class ActionCable::Channel::PeriodicTimersTest < ActiveSupport::TestCase end test "timer start and stop" do - Concurrent::TimerTask.expects(:new).times(2).returns(true) + EventMachine::PeriodicTimer.expects(:new).times(2).returns(true) channel = ChatChannel.new @connection, "{id: 1}", { id: 1 } channel.expects(:stop_periodic_timers).once diff --git a/actioncable/test/channel/stream_test.rb b/actioncable/test/channel/stream_test.rb index 947efd96d4..3fa2b291b7 100644 --- a/actioncable/test/channel/stream_test.rb +++ b/actioncable/test/channel/stream_test.rb @@ -31,7 +31,9 @@ class ActionCable::Channel::StreamTest < ActionCable::TestCase test "stream_for" do run_in_eventmachine do connection = TestConnection.new - connection.expects(:pubsub).returns mock().tap { |m| m.expects(:subscribe).with("action_cable:channel:stream_test:chat:Room#1-Campfire", kind_of(Proc), kind_of(Proc)).returns stub_everything(:pubsub) } + EM.next_tick do + connection.expects(:pubsub).returns mock().tap { |m| m.expects(:subscribe).with("action_cable:channel:stream_test:chat:Room#1-Campfire", kind_of(Proc), kind_of(Proc)).returns stub_everything(:pubsub) } + end channel = ChatChannel.new connection, "" channel.stream_for Room.new(1) @@ -39,35 +41,39 @@ class ActionCable::Channel::StreamTest < ActionCable::TestCase end test "stream_from subscription confirmation" do - run_in_eventmachine do + EM.run do connection = TestConnection.new ChatChannel.new connection, "{id: 1}", { id: 1 } assert_nil connection.last_transmission - wait_for_async + EM::Timer.new(0.1) do + expected = ActiveSupport::JSON.encode "identifier" => "{id: 1}", "type" => "confirm_subscription" + connection.transmit(expected) - expected = ActiveSupport::JSON.encode "identifier" => "{id: 1}", "type" => "confirm_subscription" - connection.transmit(expected) + assert_equal expected, connection.last_transmission, "Did not receive subscription confirmation within 0.1s" - assert_equal expected, connection.last_transmission, "Did not receive subscription confirmation within 0.1s" + EM.run_deferred_callbacks + EM.stop + end end end test "subscription confirmation should only be sent out once" do - run_in_eventmachine do + EM.run do connection = TestConnection.new channel = ChatChannel.new connection, "test_channel" channel.send_confirmation channel.send_confirmation - wait_for_async + EM.run_deferred_callbacks expected = ActiveSupport::JSON.encode "identifier" => "test_channel", "type" => "confirm_subscription" assert_equal expected, connection.last_transmission, "Did not receive subscription confirmation" assert_equal 1, connection.transmissions.size + EM.stop end end diff --git a/actioncable/test/connection/base_test.rb b/actioncable/test/connection/base_test.rb index e2b017a9a1..182562db82 100644 --- a/actioncable/test/connection/base_test.rb +++ b/actioncable/test/connection/base_test.rb @@ -37,8 +37,6 @@ class ActionCable::Connection::BaseTest < ActionCable::TestCase connection.process assert connection.websocket.possible? - - wait_for_async assert connection.websocket.alive? end end @@ -55,15 +53,16 @@ class ActionCable::Connection::BaseTest < ActionCable::TestCase test "on connection open" do run_in_eventmachine do connection = open_connection + connection.process connection.websocket.expects(:transmit).with(regexp_matches(/\_ping/)) connection.message_buffer.expects(:process!) - connection.process - wait_for_async - - assert_equal [ connection ], @server.connections - assert connection.connected + # Allow EM to run on_open callback + EM.next_tick do + assert_equal [ connection ], @server.connections + assert connection.connected + end end end @@ -73,12 +72,12 @@ class ActionCable::Connection::BaseTest < ActionCable::TestCase connection.process # Setup the connection - Concurrent::TimerTask.stubs(:new).returns(true) - connection.send :handle_open + EventMachine.stubs(:add_periodic_timer).returns(true) + connection.send :on_open assert connection.connected connection.subscriptions.expects(:unsubscribe_from_all) - connection.send :handle_close + connection.send :on_close assert ! connection.connected assert_equal [], @server.connections diff --git a/actioncable/test/connection/identifier_test.rb b/actioncable/test/connection/identifier_test.rb index 1019ad541e..a110dfdee0 100644 --- a/actioncable/test/connection/identifier_test.rb +++ b/actioncable/test/connection/identifier_test.rb @@ -68,10 +68,10 @@ class ActionCable::Connection::IdentifierTest < ActionCable::TestCase @connection = Connection.new(server, env) @connection.process - @connection.send :handle_open + @connection.send :on_open end def close_connection - @connection.send :handle_close + @connection.send :on_close end end diff --git a/actioncable/test/connection/multiple_identifiers_test.rb b/actioncable/test/connection/multiple_identifiers_test.rb index e9bb4e6d7f..55a9f96cb3 100644 --- a/actioncable/test/connection/multiple_identifiers_test.rb +++ b/actioncable/test/connection/multiple_identifiers_test.rb @@ -32,10 +32,10 @@ class ActionCable::Connection::MultipleIdentifiersTest < ActionCable::TestCase @connection = Connection.new(server, env) @connection.process - @connection.send :handle_open + @connection.send :on_open end def close_connection - @connection.send :handle_close + @connection.send :on_close end end diff --git a/actioncable/test/stubs/test_server.rb b/actioncable/test/stubs/test_server.rb index 56d132b30a..6e6541a952 100644 --- a/actioncable/test/stubs/test_server.rb +++ b/actioncable/test/stubs/test_server.rb @@ -14,7 +14,6 @@ class TestServer @config.subscription_adapter.new(self) end - def stream_event_loop - @stream_event_loop ||= ActionCable::Connection::StreamEventLoop.new + def send_async end end diff --git a/actioncable/test/subscription_adapter/common.rb b/actioncable/test/subscription_adapter/common.rb index 361858784e..d4a13be889 100644 --- a/actioncable/test/subscription_adapter/common.rb +++ b/actioncable/test/subscription_adapter/common.rb @@ -1,6 +1,7 @@ require 'test_helper' require 'concurrent' +require 'action_cable/process/logging' require 'active_support/core_ext/hash/indifferent_access' require 'pathname' @@ -23,6 +24,8 @@ module CommonSubscriptionAdapterTest # and now the "real" setup for our test: + spawn_eventmachine + server.config.cable = cable_config.with_indifferent_access adapter_klass = server.config.pubsub_adapter diff --git a/actioncable/test/test_helper.rb b/actioncable/test/test_helper.rb index 8ddbd4e764..6636ce078b 100644 --- a/actioncable/test/test_helper.rb +++ b/actioncable/test/test_helper.rb @@ -13,16 +13,28 @@ require 'rack/mock' # Require all the stubs and models Dir[File.dirname(__FILE__) + '/stubs/*.rb'].each {|file| require file } +require 'faye/websocket' +class << Faye::WebSocket + remove_method :ensure_reactor_running + + # We don't want Faye to start the EM reactor in tests because it makes testing much harder. + # We want to be able to start and stop EM loop in tests to make things simpler. + def ensure_reactor_running + # no-op + end +end + class ActionCable::TestCase < ActiveSupport::TestCase - def wait_for_async - e = Concurrent.global_io_executor - until e.completed_task_count == e.scheduled_task_count - sleep 0.1 + def run_in_eventmachine + EM.run do + yield + + EM.run_deferred_callbacks + EM.stop end end - def run_in_eventmachine - yield - wait_for_async + def spawn_eventmachine + Thread.new { EventMachine.run } unless EventMachine.reactor_running? end end diff --git a/actionpack/lib/abstract_controller/rendering.rb b/actionpack/lib/abstract_controller/rendering.rb index 63fd76d9b7..e765d73ce4 100644 --- a/actionpack/lib/abstract_controller/rendering.rb +++ b/actionpack/lib/abstract_controller/rendering.rb @@ -82,13 +82,13 @@ module AbstractController # <tt>render :file => "foo/bar"</tt>. # :api: plugin def _normalize_args(action=nil, options={}) - case action - when ActionController::Parameters - unless action.permitted? + if action.respond_to?(:permitted?) + if action.permitted? + action + else raise ArgumentError, "render parameters are not permitted" end - action - when Hash + elsif action.is_a?(Hash) action else options diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index ba03430930..d3382ef296 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -587,11 +587,11 @@ module ActionController def method_missing(method_sym, *args, &block) if @parameters.respond_to?(method_sym) message = <<-DEPRECATE.squish - Method #{ method_sym } is deprecated and will be removed in Rails 5.1, + Method #{method_sym} is deprecated and will be removed in Rails 5.1, as `ActionController::Parameters` no longer inherits from hash. Using this deprecated behavior exposes potential security problems. If you continue to use this method you may be creating - a security vulunerability in your app that can be exploited. Instead, + a security vulnerability in your app that can be exploited. Instead, consider using one of these documented methods which are not deprecated: http://api.rubyonrails.org/v#{ActionPack.version}/classes/ActionController/Parameters.html DEPRECATE diff --git a/actionpack/lib/action_dispatch/http/mime_type.rb b/actionpack/lib/action_dispatch/http/mime_type.rb index 6abbf9c754..463b5fe405 100644 --- a/actionpack/lib/action_dispatch/http/mime_type.rb +++ b/actionpack/lib/action_dispatch/http/mime_type.rb @@ -106,27 +106,33 @@ module Mime result = @index <=> item.index if result == 0 result end - - def ==(item) - @name == item.to_s - end end class AcceptList < Array #:nodoc: def assort! sort! + text_xml_idx = find_item_by_name self, 'text/xml' + app_xml_idx = find_item_by_name self, Mime[:xml].to_s + # Take care of the broken text/xml entry by renaming or deleting it if text_xml_idx && app_xml_idx + app_xml = self[app_xml_idx] + text_xml = self[text_xml_idx] + app_xml.q = [text_xml.q, app_xml.q].max # set the q value to the max of the two - exchange_xml_items if app_xml_idx > text_xml_idx # make sure app_xml is ahead of text_xml in the list + if app_xml_idx > text_xml_idx # make sure app_xml is ahead of text_xml in the list + self[app_xml_idx], self[text_xml_idx] = text_xml, app_xml + app_xml_idx, text_xml_idx = text_xml_idx, app_xml_idx + end delete_at(text_xml_idx) # delete text_xml from the list elsif text_xml_idx - text_xml.name = Mime[:xml].to_s + self[text_xml_idx].name = Mime[:xml].to_s end # Look for more specific XML-based types and sort them ahead of app/xml if app_xml_idx + app_xml = self[app_xml_idx] idx = app_xml_idx while idx < length @@ -135,7 +141,7 @@ module Mime if type.name.ends_with? '+xml' self[app_xml_idx], self[idx] = self[idx], app_xml - @app_xml_idx = idx + app_xml_idx = idx end idx += 1 end @@ -146,26 +152,9 @@ module Mime end private - def text_xml_idx - @text_xml_idx ||= index('text/xml') - end - - def app_xml_idx - @app_xml_idx ||= index(Mime[:xml].to_s) - end - - def text_xml - self[text_xml_idx] - end - - def app_xml - self[app_xml_idx] - end - - def exchange_xml_items - self[app_xml_idx], self[text_xml_idx] = text_xml, app_xml - @app_xml_idx, @text_xml_idx = text_xml_idx, app_xml_idx - end + def find_item_by_name(array, name) + array.index { |item| item.name == name } + end end class << self diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 3f569230c2..d1b9586533 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -63,12 +63,16 @@ class TestController < ActionController::Base end def dynamic_render - render params[:id] # => String, AC:Params + render params[:id] # => String, AC::Params + end + + def dynamic_render_permit + render params[:id].permit(:file) end def dynamic_render_with_file # This is extremely bad, but should be possible to do. - file = params[:id] # => String, AC:Params + file = params[:id] # => String, AC::Params render file: file end @@ -253,6 +257,11 @@ end class ExpiresInRenderTest < ActionController::TestCase tests TestController + def setup + super + ActionController::Base.view_paths.paths.each(&:clear_cache) + end + def test_dynamic_render_with_file # This is extremely bad, but should be possible to do. assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) @@ -268,6 +277,14 @@ class ExpiresInRenderTest < ActionController::TestCase end end + def test_permitted_dynamic_render_file_hash + skip "FIXME: this test passes on 4-2-stable but not master. Why?" + assert File.exist?(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')) + response = get :dynamic_render_permit, params: { id: { file: '../\\../test/abstract_unit.rb' } } + assert_equal File.read(File.join(File.dirname(__FILE__), '../../test/abstract_unit.rb')), + response.body + end + def test_dynamic_render_file_hash assert_raises ArgumentError do get :dynamic_render, params: { id: { file: '../\\../test/abstract_unit.rb' } } diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index 98ac2c1c22..d85681e0d1 100644 --- a/actionview/CHANGELOG.md +++ b/actionview/CHANGELOG.md @@ -1,3 +1,8 @@ +* Fix stripping the digest from the automatically generated img tag alt + attribute when assets are handled by Sprockets >=3.0. + + *Bart de Water* + * Create a new `ActiveSupport::SafeBuffer` instance when `content_for` is flushed. Fixes #19890 diff --git a/actionview/lib/action_view/helpers/asset_tag_helper.rb b/actionview/lib/action_view/helpers/asset_tag_helper.rb index cc54faa778..413c35954c 100644 --- a/actionview/lib/action_view/helpers/asset_tag_helper.rb +++ b/actionview/lib/action_view/helpers/asset_tag_helper.rb @@ -239,7 +239,7 @@ module ActionView # image_alt('underscored_file_name.png') # # => Underscored file name def image_alt(src) - File.basename(src, '.*'.freeze).sub(/-[[:xdigit:]]{32}\z/, ''.freeze).tr('-_'.freeze, ' '.freeze).capitalize + File.basename(src, '.*'.freeze).sub(/-[[:xdigit:]]{32,64}\z/, ''.freeze).tr('-_'.freeze, ' '.freeze).capitalize end # Returns an HTML video tag for the +sources+. If +sources+ is a string, diff --git a/actionview/lib/action_view/helpers/tags/collection_helpers.rb b/actionview/lib/action_view/helpers/tags/collection_helpers.rb index 1d3b1ecf0b..fb51460c8e 100644 --- a/actionview/lib/action_view/helpers/tags/collection_helpers.rb +++ b/actionview/lib/action_view/helpers/tags/collection_helpers.rb @@ -94,7 +94,7 @@ module ActionView end end - # Append a hidden field to make sure something will be sent back to the + # Prepend a hidden field to make sure something will be sent back to the # server if all radio buttons are unchecked. if options.fetch('include_hidden', true) hidden_field + rendered_collection diff --git a/actionview/test/template/asset_tag_helper_test.rb b/actionview/test/template/asset_tag_helper_test.rb index 8592a2a083..8bfd19eb26 100644 --- a/actionview/test/template/asset_tag_helper_test.rb +++ b/actionview/test/template/asset_tag_helper_test.rb @@ -459,6 +459,7 @@ class AssetTagHelperTest < ActionView::TestCase [nil, '/', '/foo/bar/', 'foo/bar/'].each do |prefix| assert_equal 'Rails', image_alt("#{prefix}rails.png") assert_equal 'Rails', image_alt("#{prefix}rails-9c0a079bdd7701d7e729bd956823d153.png") + assert_equal 'Rails', image_alt("#{prefix}rails-f56ef62bc41b040664e801a38f068082a75d506d9048307e8096737463503d0b.png") assert_equal 'Long file name with hyphens', image_alt("#{prefix}long-file-name-with-hyphens.png") assert_equal 'Long file name with underscores', image_alt("#{prefix}long_file_name_with_underscores.png") end diff --git a/actionview/test/template/text_helper_test.rb b/actionview/test/template/text_helper_test.rb index fb98ac6330..03c7597505 100644 --- a/actionview/test/template/text_helper_test.rb +++ b/actionview/test/template/text_helper_test.rb @@ -43,11 +43,11 @@ class TextHelperTest < ActionView::TestCase end def test_simple_format_should_sanitize_input_when_sanitize_option_is_not_false - assert_equal "<p><b> test with unsafe string </b></p>", simple_format("<b> test with unsafe string </b><script>code!</script>") + assert_equal "<p><b> test with unsafe string </b>code!</p>", simple_format("<b> test with unsafe string </b><script>code!</script>") end def test_simple_format_should_sanitize_input_when_sanitize_option_is_true - assert_equal '<p><b> test with unsafe string </b></p>', + assert_equal '<p><b> test with unsafe string </b>code!</p>', simple_format('<b> test with unsafe string </b><script>code!</script>', {}, sanitize: true) end @@ -198,7 +198,7 @@ class TextHelperTest < ActionView::TestCase def test_highlight_should_sanitize_input assert_equal( - "This is a <mark>beautiful</mark> morning", + "This is a <mark>beautiful</mark> morningcode!", highlight("This is a beautiful morning<script>code!</script>", "beautiful") ) end diff --git a/activejob/lib/active_job/queue_adapters.rb b/activejob/lib/active_job/queue_adapters.rb index aeb1fe1e73..2c5039ef4d 100644 --- a/activejob/lib/active_job/queue_adapters.rb +++ b/activejob/lib/active_job/queue_adapters.rb @@ -27,7 +27,7 @@ module ActiveJob # | Resque | Yes | Yes | Yes (Gem) | Queue | Global | Yes | # | Sidekiq | Yes | Yes | Yes | Queue | No | Job | # | Sneakers | Yes | Yes | No | Queue | Queue | No | - # | Sucker Punch | Yes | Yes | No | No | No | No | + # | Sucker Punch | Yes | Yes | Yes | No | No | No | # | Active Job Async | Yes | Yes | Yes | No | No | No | # | Active Job Inline | No | Yes | N/A | N/A | N/A | N/A | # diff --git a/activejob/lib/active_job/queue_adapters/sucker_punch_adapter.rb b/activejob/lib/active_job/queue_adapters/sucker_punch_adapter.rb index c6c35f8ab4..311109e958 100644 --- a/activejob/lib/active_job/queue_adapters/sucker_punch_adapter.rb +++ b/activejob/lib/active_job/queue_adapters/sucker_punch_adapter.rb @@ -5,12 +5,10 @@ module ActiveJob # == Sucker Punch adapter for Active Job # # Sucker Punch is a single-process Ruby asynchronous processing library. - # It's girl_friday and DSL sugar on top of Celluloid. With Celluloid's - # actor pattern, we can do asynchronous processing within a single process. - # This reduces costs of hosting on a service like Heroku along with the - # memory footprint of having to maintain additional jobs if hosting on - # a dedicated server. All queues can run within a single Rails/Sinatra - # process. + # This reduces the cost of of hosting on a service like Heroku along + # with the memory footprint of having to maintain additional jobs if + # hosting on a dedicated server. All queues can run within a + # single application (eg. Rails, Sinatra, etc.) process. # # Read more about Sucker Punch {here}[https://github.com/brandonhilkert/sucker_punch]. # @@ -19,11 +17,22 @@ module ActiveJob # Rails.application.config.active_job.queue_adapter = :sucker_punch class SuckerPunchAdapter def enqueue(job) #:nodoc: - JobWrapper.new.async.perform job.serialize + if JobWrapper.respond_to?(:perform_async) + # sucker_punch 2.0 API + JobWrapper.perform_async job.serialize + else + # sucker_punch 1.0 API + JobWrapper.new.async.perform job.serialize + end end def enqueue_at(job, timestamp) #:nodoc: - raise NotImplementedError, "This queueing backend does not support scheduling jobs. To see what features are supported go to http://api.rubyonrails.org/classes/ActiveJob/QueueAdapters.html" + if JobWrapper.respond_to?(:perform_in) + delay = timestamp - Time.current.to_f + JobWrapper.perform_in delay, job.serialize + else + raise NotImplementedError, 'sucker_punch 1.0 does not support `enqueued_at`. Please upgrade to version ~> 2.0.0 to enable this behavior.' + end end class JobWrapper #:nodoc: diff --git a/activejob/test/support/integration/adapters/sidekiq.rb b/activejob/test/support/integration/adapters/sidekiq.rb index 9aa07bcb52..2f19d7dacc 100644 --- a/activejob/test/support/integration/adapters/sidekiq.rb +++ b/activejob/test/support/integration/adapters/sidekiq.rb @@ -26,7 +26,7 @@ module SidekiqJobsManager continue_read.close death_write.close - # Celluloid & Sidekiq are not warning-clean :( + # Sidekiq is not warning-clean :( $VERBOSE = false $stdin.reopen('/dev/null') @@ -49,8 +49,6 @@ module SidekiqJobsManager self_write.puts("TERM") end - require 'celluloid' - Celluloid.logger = nil require 'sidekiq/launcher' sidekiq = Sidekiq::Launcher.new({queues: ["integration_tests"], environment: "test", diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index f3dc26ddb3..50a0b291b3 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,22 @@ +* `ActiveRecord::Relation#reverse_order` throws `ActiveRecord::IrreversibleOrderError` + when the order can not be reversed using current trivial algorithm. + Also raises the same error when `#reverse_order` is called on + relation without any order and table has no primary key: + + Topic.order("concat(author_name, title)").reverse_order + # Before: SELECT `topics`.* FROM `topics` ORDER BY concat(author_name DESC, title) DESC + # After: raises ActiveRecord::IrreversibleOrderError + Edge.all.reverse_order + # Before: SELECT `edges`.* FROM `edges` ORDER BY `edges`.`` DESC + # After: raises ActiveRecord::IrreversibleOrderError + + *Bogdan Gusiev* + +* Improve schema_migrations insertion performance by inserting all versions + in one INSERT SQL. + + *Akira Matsuda*, *Naoto Koshikawa* + * Using `references` or `belongs_to` in migrations will always add index for the referenced column by default, without adding `index: true` option to generated migration file. Users can opt out of this by passing diff --git a/activerecord/lib/active_record/aggregations.rb b/activerecord/lib/active_record/aggregations.rb index be88c7c9e8..3ff41ed81b 100644 --- a/activerecord/lib/active_record/aggregations.rb +++ b/activerecord/lib/active_record/aggregations.rb @@ -33,7 +33,7 @@ module ActiveRecord # the database). # # class Customer < ActiveRecord::Base - # composed_of :balance, class_name: "Money", mapping: %w(balance amount) + # composed_of :balance, class_name: "Money", mapping: %w(amount currency) # composed_of :address, mapping: [ %w(address_street street), %w(address_city city) ] # end # 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 70868ebd03..002f2ea8ce 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/schema_statements.rb @@ -957,9 +957,9 @@ module ActiveRecord def dump_schema_information #:nodoc: sm_table = ActiveRecord::Migrator.schema_migrations_table_name - ActiveRecord::SchemaMigration.order('version').map { |sm| - "INSERT INTO #{sm_table} (version) VALUES ('#{sm.version}');" - }.join "\n\n" + sql = "INSERT INTO #{sm_table} (version) VALUES " + sql << ActiveRecord::SchemaMigration.order('version').pluck(:version).map {|v| "('#{v}')" }.join(', ') + sql << ";\n\n" end # Should not be called normally, but this operation is non-destructive. @@ -987,14 +987,12 @@ module ActiveRecord execute "INSERT INTO #{sm_table} (version) VALUES ('#{version}')" end - inserted = Set.new - (versions - migrated).each do |v| - if inserted.include?(v) - raise "Duplicate migration #{v}. Please renumber your migrations to resolve the conflict." - elsif v < version - execute "INSERT INTO #{sm_table} (version) VALUES ('#{v}')" - inserted << v + inserting = (versions - migrated).select {|v| v < version} + if inserting.any? + if (duplicate = inserting.detect {|v| inserting.count(v) > 1}) + raise "Duplicate migration #{duplicate}. Please renumber your migrations to resolve the conflict." end + execute "INSERT INTO #{sm_table} (version) VALUES #{inserting.map {|v| '(#{v})'}.join(', ') }" end end diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb index e5906b6756..87f32c042c 100644 --- a/activerecord/lib/active_record/errors.rb +++ b/activerecord/lib/active_record/errors.rb @@ -275,4 +275,9 @@ module ActiveRecord # The mysql2 and postgresql adapters support setting the transaction isolation level. class TransactionIsolationError < ActiveRecordError end + + # IrreversibleOrderError is raised when a relation's order is too complex for + # +reverse_order+ to automatically reverse. + class IrreversibleOrderError < ActiveRecordError + end end diff --git a/activerecord/lib/active_record/inheritance.rb b/activerecord/lib/active_record/inheritance.rb index 3a17f74b1d..3b6fb70d0d 100644 --- a/activerecord/lib/active_record/inheritance.rb +++ b/activerecord/lib/active_record/inheritance.rb @@ -52,7 +52,11 @@ module ActiveRecord attrs = args.first if has_attribute?(inheritance_column) - subclass = subclass_from_attributes(attrs) || subclass_from_attributes(column_defaults) + subclass = subclass_from_attributes(attrs) + + if subclass.nil? && base_class == self + subclass = subclass_from_attributes(column_defaults) + end end if subclass && subclass != self diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 522c35252f..4d661735cc 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -102,10 +102,10 @@ module ActiveRecord # Saves the model. # - # If the model is new a record gets created in the database, otherwise + # If the model is new, a record gets created in the database, otherwise # the existing record gets updated. # - # By default, save always run validations. If any of them fail the action + # By default, save always runs validations. If any of them fail the action # is cancelled and #save returns +false+. However, if you supply # validate: false, validations are bypassed altogether. See # ActiveRecord::Validations for more information. @@ -132,9 +132,10 @@ module ActiveRecord # If the model is new, a record gets created in the database, otherwise # the existing record gets updated. # - # With #save! validations always run. If any of them fail - # ActiveRecord::RecordInvalid gets raised. See ActiveRecord::Validations - # for more information. + # By default, #save! always runs validations. If any of them fail + # ActiveRecord::RecordInvalid gets raised. However, if you supply + # validate: false, validations are bypassed altogether. See + # ActiveRecord::Validations for more information. # # By default, #save! also sets the +updated_at+/+updated_on+ attributes to # the current time. However, if you supply <tt>touch: false</tt>, these @@ -246,7 +247,7 @@ module ActiveRecord # This method raises an ActiveRecord::ActiveRecordError if the # attribute is marked as readonly. # - # See also #update_column. + # Also see #update_column. def update_attribute(name, value) name = name.to_s verify_readonly_attribute(name) @@ -348,7 +349,7 @@ module ActiveRecord end # Wrapper around #decrement that saves the record. This method differs from - # its non-bang version in that it passes through the attribute setter. + # its non-bang version in the sense that it passes through the attribute setter. # Saving is not subjected to validation checks. Returns +true+ if the # record could be saved. def decrement!(attribute, by = 1) @@ -373,7 +374,7 @@ module ActiveRecord end # Wrapper around #toggle that saves the record. This method differs from - # its non-bang version in that it passes through the attribute setter. + # its non-bang version in the sense that it passes through the attribute setter. # Saving is not subjected to validation checks. Returns +true+ if the # record could be saved. def toggle!(attribute) diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb index 3cbb12a09d..3f5d6de78a 100644 --- a/activerecord/lib/active_record/relation/finder_methods.rb +++ b/activerecord/lib/active_record/relation/finder_methods.rb @@ -489,20 +489,19 @@ module ActiveRecord end def find_nth(index, offset = nil) + # TODO: once the offset argument is removed we rely on offset_index + # within find_nth_with_limit, rather than pass it in via + # find_nth_with_limit_and_offset + if offset + ActiveSupport::Deprecation.warn(<<-MSG.squish) + Passing an offset argument to find_nth is deprecated, + please use Relation#offset instead. + MSG + end if loaded? @records[index] else - # TODO: once the offset argument is removed we rely on offset_index - # within find_nth_with_limit, rather than pass it in via - # find_nth_with_limit_and_offset - if offset - ActiveSupport::Deprecation.warn(<<-MSG.squish) - Passing an offset argument to find_nth is deprecated, - please use Relation#offset instead. - MSG - else - offset = offset_index - end + offset ||= offset_index @offsets[offset + index] ||= find_nth_with_limit_and_offset(index, 1, offset: offset).first end end diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 716b1e8505..8ef9f9f627 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -1104,14 +1104,21 @@ module ActiveRecord end def reverse_sql_order(order_query) - order_query = ["#{quoted_table_name}.#{quoted_primary_key} ASC"] if order_query.empty? + if order_query.empty? + return [table[primary_key].desc] if primary_key + raise IrreversibleOrderError, + "Relation has no current order and table has no primary key to be used as default order" + end order_query.flat_map do |o| case o when Arel::Nodes::Ordering o.reverse when String - o.to_s.split(',').map! do |s| + if does_not_support_reverse?(o) + raise IrreversibleOrderError, "Order #{o.inspect} can not be reversed automatically" + end + o.split(',').map! do |s| s.strip! s.gsub!(/\sasc\Z/i, ' DESC') || s.gsub!(/\sdesc\Z/i, ' ASC') || s.concat(' DESC') end @@ -1121,6 +1128,13 @@ module ActiveRecord end end + def does_not_support_reverse?(order) + #uses sql function with multiple arguments + order =~ /\([^()]*,[^()]*\)/ || + # uses "nulls first" like construction + order =~ /nulls (first|last)\Z/i + end + def build_order(arel) orders = order_values.uniq orders.reject!(&:blank?) diff --git a/activerecord/test/cases/inheritance_test.rb b/activerecord/test/cases/inheritance_test.rb index c870247a4a..7da6842047 100644 --- a/activerecord/test/cases/inheritance_test.rb +++ b/activerecord/test/cases/inheritance_test.rb @@ -493,6 +493,10 @@ class InheritanceComputeTypeTest < ActiveRecord::TestCase assert_equal 'Firm', firm.type assert_instance_of Firm, firm + client = Client.new + assert_equal 'Client', client.type + assert_instance_of Client, client + firm = Company.new(type: 'Client') # overwrite the default type assert_equal 'Client', firm.type assert_instance_of Client, firm diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb index 0638edacbd..090b885dd5 100644 --- a/activerecord/test/cases/relations_test.rb +++ b/activerecord/test/cases/relations_test.rb @@ -19,6 +19,7 @@ require 'models/aircraft' require "models/possession" require "models/reader" require "models/categorization" +require "models/edge" class RelationTest < ActiveRecord::TestCase fixtures :authors, :topics, :entrants, :developers, :companies, :developers_projects, :accounts, :categories, :categorizations, :posts, :comments, @@ -223,6 +224,39 @@ class RelationTest < ActiveRecord::TestCase assert_equal topics(:fifth).title, topics.first.title end + def test_reverse_order_with_function + topics = Topic.order("length(title)").reverse_order + assert_equal topics(:second).title, topics.first.title + end + + def test_reverse_order_with_function_other_predicates + topics = Topic.order("author_name, length(title), id").reverse_order + assert_equal topics(:second).title, topics.first.title + topics = Topic.order("length(author_name), id, length(title)").reverse_order + assert_equal topics(:fifth).title, topics.first.title + end + + def test_reverse_order_with_multiargument_function + assert_raises(ActiveRecord::IrreversibleOrderError) do + Topic.order("concat(author_name, title)").reverse_order + end + end + + def test_reverse_order_with_nulls_first_or_last + assert_raises(ActiveRecord::IrreversibleOrderError) do + Topic.order("title NULLS FIRST").reverse_order + end + assert_raises(ActiveRecord::IrreversibleOrderError) do + Topic.order("title nulls last").reverse_order + end + end + + def test_default_reverse_order_on_table_without_primary_key + assert_raises(ActiveRecord::IrreversibleOrderError) do + Edge.all.reverse_order + end + end + def test_order_with_hash_and_symbol_generates_the_same_sql assert_equal Topic.order(:id).to_sql, Topic.order(:id => :asc).to_sql end diff --git a/activesupport/lib/active_support/core_ext/module/deprecation.rb b/activesupport/lib/active_support/core_ext/module/deprecation.rb index 56d670fbe8..f3f2e7f5fc 100644 --- a/activesupport/lib/active_support/core_ext/module/deprecation.rb +++ b/activesupport/lib/active_support/core_ext/module/deprecation.rb @@ -13,8 +13,8 @@ class Module # # class MyLib::Deprecator # def deprecation_warning(deprecated_method_name, message, caller_backtrace = nil) - # message = "#{deprecated_method_name} is deprecated and will be removed from MyLibrary | #{message}" - # Kernel.warn message + # message = "#{deprecated_method_name} is deprecated and will be removed from MyLibrary | #{message}" + # Kernel.warn message # end # end def deprecate(*method_names) diff --git a/guides/source/active_record_querying.md b/guides/source/active_record_querying.md index 784be91845..63658e7c8b 100644 --- a/guides/source/active_record_querying.md +++ b/guides/source/active_record_querying.md @@ -1085,6 +1085,8 @@ SELECT categories.* FROM categories INNER JOIN tags ON tags.article_id = articles.id ``` +Or, in English: "return all categories that have articles, where those articles have a comment made by a guest, and where those articles also have a tag." + #### Specifying Conditions on the Joined Tables You can specify conditions on the joined tables using the regular [Array](#array-conditions) and [String](#pure-string-conditions) conditions. [Hash conditions](#hash-conditions) provide a special syntax for specifying conditions for the joined tables: diff --git a/guides/source/asset_pipeline.md b/guides/source/asset_pipeline.md index 60f78d011c..5bdaf600ad 100644 --- a/guides/source/asset_pipeline.md +++ b/guides/source/asset_pipeline.md @@ -45,7 +45,7 @@ gem 'coffee-rails' ``` Using the `--skip-sprockets` option will prevent Rails 4 from adding -`sass-rails` and `uglifier` to Gemfile, so if you later want to enable +`sass-rails` and `uglifier` to your Gemfile, so if you later want to enable the asset pipeline you will have to add those gems to your Gemfile. Also, creating an application with the `--skip-sprockets` option will generate a slightly different `config/application.rb` file, with a require statement @@ -66,7 +66,7 @@ config.assets.js_compressor = :uglifier ``` NOTE: The `sass-rails` gem is automatically used for CSS compression if included -in Gemfile and no `config.assets.css_compressor` option is set. +in the Gemfile and no `config.assets.css_compressor` option is set. ### Main Features @@ -1300,7 +1300,7 @@ Rails 4 no longer sets default config values for Sprockets in `test.rb`, so environment are: `config.assets.compile = true`, `config.assets.compress = false`, `config.assets.debug = false` and `config.assets.digest = false`. -The following should also be added to `Gemfile`: +The following should also be added to your `Gemfile`: ```ruby gem 'sass-rails', "~> 3.2.3" diff --git a/guides/source/upgrading_ruby_on_rails.md b/guides/source/upgrading_ruby_on_rails.md index 936547a8cc..202e5b5cb9 100644 --- a/guides/source/upgrading_ruby_on_rails.md +++ b/guides/source/upgrading_ruby_on_rails.md @@ -57,10 +57,6 @@ Upgrading from Rails 4.2 to Rails 5.0 ToDo... -### Ruby 2.2.2+ - -ToDo... - ### Active Record models now inherit from ApplicationRecord by default In Rails 4.2 an Active Record model inherits from `ActiveRecord::Base`. In Rails 5.0, diff --git a/railties/lib/rails/generators/rails/app/templates/app/channels/application_cable/channel.rb b/railties/lib/rails/generators/rails/app/templates/app/channels/application_cable/channel.rb index 438c84154d..d56fa30f4d 100644 --- a/railties/lib/rails/generators/rails/app/templates/app/channels/application_cable/channel.rb +++ b/railties/lib/rails/generators/rails/app/templates/app/channels/application_cable/channel.rb @@ -1,4 +1,4 @@ -# Be sure to restart your server when you modify this file. Action Cable runs in an EventMachine loop that does not support auto reloading. +# Be sure to restart your server when you modify this file. Action Cable runs in a loop that does not support auto reloading. module ApplicationCable class Channel < ActionCable::Channel::Base end diff --git a/railties/lib/rails/generators/rails/app/templates/app/channels/application_cable/connection.rb b/railties/lib/rails/generators/rails/app/templates/app/channels/application_cable/connection.rb index 965046f3c7..b4f41389ad 100644 --- a/railties/lib/rails/generators/rails/app/templates/app/channels/application_cable/connection.rb +++ b/railties/lib/rails/generators/rails/app/templates/app/channels/application_cable/connection.rb @@ -1,4 +1,4 @@ -# Be sure to restart your server when you modify this file. Action Cable runs in an EventMachine loop that does not support auto reloading. +# Be sure to restart your server when you modify this file. Action Cable runs in a loop that does not support auto reloading. module ApplicationCable class Connection < ActionCable::Connection::Base end diff --git a/railties/lib/rails/generators/rails/app/templates/config.ru.tt b/railties/lib/rails/generators/rails/app/templates/config.ru.tt index 343c0833d7..70556fcc99 100644 --- a/railties/lib/rails/generators/rails/app/templates/config.ru.tt +++ b/railties/lib/rails/generators/rails/app/templates/config.ru.tt @@ -3,8 +3,9 @@ require ::File.expand_path('../config/environment', __FILE__) <%- unless options[:skip_action_cable] -%> -# Action Cable requires that all classes are loaded in advance +# Action Cable uses EventMachine which requires that all classes are loaded in advance Rails.application.eager_load! +require 'action_cable/process/logging' <%- end -%> run Rails.application |