From 608207b2e5c4ed934a925db98e201519f798efce Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 27 Nov 2017 17:43:24 +1100 Subject: [PATCH] FEATURE: avatar proxy happens in background This ensures that even if it is slow to download avatars site will continue to work Also simplifies hijack pattern --- app/controllers/user_avatars_controller.rb | 52 ++++++------ lib/hijack.rb | 96 ++++++---------------- spec/components/hijack_spec.rb | 34 ++++---- 3 files changed, 73 insertions(+), 109 deletions(-) diff --git a/app/controllers/user_avatars_controller.rb b/app/controllers/user_avatars_controller.rb index ee0c08bffc6..ae93f484310 100644 --- a/app/controllers/user_avatars_controller.rb +++ b/app/controllers/user_avatars_controller.rb @@ -9,13 +9,15 @@ class UserAvatarsController < ApplicationController guardian.ensure_can_edit!(user) if user - user.create_user_avatar(user_id: user.id) unless user.user_avatar - user.user_avatar.update_gravatar! + hijack do + user.create_user_avatar(user_id: user.id) unless user.user_avatar + user.user_avatar.update_gravatar! - render json: { - gravatar_upload_id: user.user_avatar.gravatar_upload_id, - gravatar_avatar_template: User.avatar_template(user.username, user.user_avatar.gravatar_upload_id) - } + render json: { + gravatar_upload_id: user.user_avatar.gravatar_upload_id, + gravatar_avatar_template: User.avatar_template(user.username, user.user_avatar.gravatar_upload_id) + } + end else raise Discourse::NotFound end @@ -31,17 +33,17 @@ class UserAvatarsController < ApplicationController params.require(:version) params.require(:size) - no_cookies + hijack do + identity = LetterAvatar::Identity.new + identity.letter = params[:letter].to_s[0].upcase + identity.color = params[:color].scan(/../).map(&:hex) + image = LetterAvatar.generate(params[:letter].to_s, params[:size].to_i, identity: identity) - identity = LetterAvatar::Identity.new - identity.letter = params[:letter].to_s[0].upcase - identity.color = params[:color].scan(/../).map(&:hex) - image = LetterAvatar.generate(params[:letter].to_s, params[:size].to_i, identity: identity) - - response.headers["Last-Modified"] = File.ctime(image).httpdate - response.headers["Content-Length"] = File.size(image).to_s - immutable_for(1.year) - send_file image, disposition: nil + response.headers["Last-Modified"] = File.ctime(image).httpdate + response.headers["Content-Length"] = File.size(image).to_s + immutable_for(1.year) + send_file image, disposition: nil + end end def show_letter @@ -53,20 +55,22 @@ class UserAvatarsController < ApplicationController return render_blank if params[:version] != LetterAvatar.version - image = LetterAvatar.generate(params[:username].to_s, params[:size].to_i) + hijack do + image = LetterAvatar.generate(params[:username].to_s, params[:size].to_i) - response.headers["Last-Modified"] = File.ctime(image).httpdate - response.headers["Content-Length"] = File.size(image).to_s - immutable_for(1.year) - send_file image, disposition: nil + response.headers["Last-Modified"] = File.ctime(image).httpdate + response.headers["Content-Length"] = File.size(image).to_s + immutable_for(1.year) + send_file image, disposition: nil + end end def show - no_cookies - # we need multisite support to keep a single origin pull for CDNs RailsMultisite::ConnectionManagement.with_hostname(params[:hostname]) do - show_in_site(params[:hostname]) + hijack do + show_in_site(params[:hostname]) + end end end diff --git a/lib/hijack.rb b/lib/hijack.rb index c1b3e37432b..6f7b7e9f2d1 100644 --- a/lib/hijack.rb +++ b/lib/hijack.rb @@ -5,58 +5,10 @@ # free up a unicorn worker while the remote IO is happening module Hijack - class FakeResponse - attr_reader :headers - def initialize - @headers = {} - end - end - - class Binder - attr_reader :content_type, :body, :status, :response - - def initialize - @content_type = 'text/plain' - @status = 500 - @body = "" - @response = FakeResponse.new - end - - def immutable_for(duration) - response.headers['Cache-Control'] = "max-age=#{duration}, public, immutable" - end - - def render(opts) - if opts[:status] - @status = opts[:status].to_i - else - @status = 200 - end - - if opts.key?(:body) - @body = opts[:body].to_s - end - - if opts.key?(:content_type) - @content_type = opts[:content_type] - end - - if opts.key?(:plain) - @content_type = 'text/plain; charset=utf-8' - @body = opts[:plain].to_s - end - - if opts.key?(:json) - @content_type = 'application/json; charset=utf-8' - @body = opts[:json] - unless String === @body - @body = @body.to_json - end - end - end - end - def hijack(&blk) + controller_class = self.class + request = self.request + if hijack = request.env['rack.hijack'] io = hijack.call @@ -67,26 +19,41 @@ module Hijack # before doing any work io.write "HTTP/1.1 " - binder = Binder.new + # this trick avoids double render, also avoids any litter that the controller hooks + # place on the response + instance = controller_class.new + response = ActionDispatch::Response.new + instance.response = response + instance.request = request + begin - binder.instance_eval(&blk) + instance.instance_eval(&blk) rescue => e Rails.logger.warn("Failed to process hijacked response correctly #{e}") end - headers = binder.response.headers - headers['Content-Length'] = binder.body.bytesize - headers['Content-Type'] = binder.content_type + unless instance.response_body + instance.status = 500 + end + + response.commit! + + body = response.body + + headers = response.headers + headers['Content-Length'] = body.bytesize + headers['Content-Type'] = response.content_type || "text/plain" headers['Connection'] = "close" - io.write "#{binder.status} OK\r\n" + status_string = Rack::Utils::HTTP_STATUS_CODES[instance.status.to_i] || "Unknown" + io.write "#{instance.status} #{status_string}\r\n" headers.each do |name, val| io.write "#{name}: #{val}\r\n" end io.write "\r\n" - io.write binder.body + io.write body io.close rescue Errno::EPIPE, IOError # happens if client terminated before we responded, ignore @@ -95,18 +62,7 @@ module Hijack # not leaked out, we use 418 ... I am a teapot to denote that we are hijacked render plain: "", status: 418 else - binder = Binder.new - binder.instance_eval(&blk) - - binder.response.headers.each do |name, val| - response.headers[name] = val - end - - render( - body: binder.body, - content_type: binder.content_type, - status: binder.status - ) + blk.call end end end diff --git a/spec/components/hijack_spec.rb b/spec/components/hijack_spec.rb index fcb6e3d069c..15c0078faec 100644 --- a/spec/components/hijack_spec.rb +++ b/spec/components/hijack_spec.rb @@ -1,37 +1,41 @@ require 'rails_helper' describe Hijack do - class Hijack::Tester + class Hijack::Tester < ApplicationController attr_reader :io include Hijack + def initialize @io = StringIO.new - end - - def hijack_test(&blk) - hijack do - self.instance_eval(&blk) - end - end - - def request - @req ||= ActionController::TestRequest.new( + self.request = ActionController::TestRequest.new( { "rack.hijack" => lambda { @io } }, nil, nil ) + # we need this for the 418 + self.response = ActionDispatch::Response.new end - def render(*opts) - # don't care + def hijack_test(&blk) + hijack(&blk) end + end let :tester do Hijack::Tester.new end + it "handles expires_in" do + tester.hijack_test do + expires_in 1.year + render body: "hello world", status: 402 + end + + expect(tester.io.string).to include("max-age=31556952") + end + it "renders non 200 status if asked for" do tester.hijack_test do render body: "hello world", status: 402 @@ -46,14 +50,14 @@ describe Hijack do render plain: "hello world" end - result = "HTTP/1.1 200 OK\r\nContent-Length: 11\r\nContent-Type: text/plain; charset=utf-8\r\nConnection: close\r\n\r\nhello world" + result = "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nContent-Length: 11\r\nConnection: close\r\n\r\nhello world" expect(tester.io.string).to eq(result) end it "returns 500 by default" do tester.hijack_test - expected = "HTTP/1.1 500 OK\r\nContent-Length: 0\r\nContent-Type: text/plain\r\nConnection: close\r\n\r\n" + expected = "HTTP/1.1 500 Internal Server Error\r\nContent-Type: text/html\r\nContent-Length: 0\r\nConnection: close\r\n\r\n" expect(tester.io.string).to eq(expected) end