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

@@ -9,6 +9,7 @@ class UserAvatarsController < ApplicationController
     guardian.ensure_can_edit!(user)
 
     if user
+      hijack do
         user.create_user_avatar(user_id: user.id) unless user.user_avatar
         user.user_avatar.update_gravatar!
 
@@ -16,6 +17,7 @@ class UserAvatarsController < ApplicationController
           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,8 +33,7 @@ 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)
@@ -43,6 +44,7 @@ class UserAvatarsController < ApplicationController
       immutable_for(1.year)
       send_file image, disposition: nil
     end
+  end
 
   def show_letter
     params.require(:username)
@@ -53,6 +55,7 @@ class UserAvatarsController < ApplicationController
 
     return render_blank if params[:version] != LetterAvatar.version
 
+    hijack do
       image = LetterAvatar.generate(params[:username].to_s, params[:size].to_i)
 
       response.headers["Last-Modified"] = File.ctime(image).httpdate
@@ -60,15 +63,16 @@ class UserAvatarsController < ApplicationController
       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
+      hijack do
         show_in_site(params[:hostname])
       end
     end
+  end
 
   protected
 

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
+          unless instance.response_body
-          headers['Content-Length'] = binder.body.bytesize
+            instance.status = 500
-          headers['Content-Type'] = binder.content_type
+          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
+      blk.call
-      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
-      )
     end
   end
 end

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
+      self.request = ActionController::TestRequest.new(
-
-    def hijack_test(&blk)
-      hijack do
-        self.instance_eval(&blk)
-      end
-    end
-
-    def request
-      @req ||= ActionController::TestRequest.new(
         { "rack.hijack" => lambda { @io } },
         nil,
         nil
       )
+      # we need this for the 418
+      self.response = ActionDispatch::Response.new
     end
 
-    def render(*opts)
+    def hijack_test(&blk)
-      # don't care
+      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