Skip to content

Commit

Permalink
Replaces thread storage with request_store
Browse files Browse the repository at this point in the history
Fixes issue roidrage#197. Under load it was possible for a thread to handle
a second request without clearing the variables in the thread. This
caused log lines to have the incorrect values for `: lograge_location`.

@abrisse suggested we used `request_store`, after a brief examination
of this gem it seems that it’s well maintained and it solves our
problem.
  • Loading branch information
leonelgalan committed Aug 3, 2017
1 parent 44d0cb0 commit b6ed2a8
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 13 deletions.
15 changes: 8 additions & 7 deletions lib/lograge/log_subscriber.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
require 'action_pack'
require 'active_support/core_ext/class/attribute'
require 'active_support/log_subscriber'
require 'request_store'

module Lograge
class RequestLogSubscriber < ActiveSupport::LogSubscriber
Expand All @@ -16,12 +17,12 @@ def process_action(event)
end

def redirect_to(event)
Thread.current[:lograge_location] = event.payload[:location]
RequestStore.store[:lograge_location] = event.payload[:location]
end

def unpermitted_parameters(event)
Thread.current[:lograge_unpermitted_params] ||= []
Thread.current[:lograge_unpermitted_params].concat(event.payload[:keys])
RequestStore.store[:lograge_unpermitted_params] ||= []
RequestStore.store[:lograge_unpermitted_params].concat(event.payload[:keys])
end

def logger
Expand Down Expand Up @@ -98,18 +99,18 @@ def extract_runtimes(event, payload)
end

def extract_location
location = Thread.current[:lograge_location]
location = RequestStore.store[:lograge_location]
return {} unless location

Thread.current[:lograge_location] = nil
RequestStore.store[:lograge_location] = nil
{ location: location }
end

def extract_unpermitted_params
unpermitted_params = Thread.current[:lograge_unpermitted_params]
unpermitted_params = RequestStore.store[:lograge_unpermitted_params]
return {} unless unpermitted_params

Thread.current[:lograge_unpermitted_params] = nil
RequestStore.store[:lograge_unpermitted_params] = nil
{ unpermitted_params: unpermitted_params }
end
end
Expand Down
1 change: 1 addition & 0 deletions lograge.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,5 @@ Gem::Specification.new do |s|
s.add_runtime_dependency 'activesupport', '>= 4', '< 5.2'
s.add_runtime_dependency 'actionpack', '>= 4', '< 5.2'
s.add_runtime_dependency 'railties', '>= 4', '< 5.2'
s.add_runtime_dependency 'request_store', '~> 1.0'
end
12 changes: 6 additions & 6 deletions spec/lograge_logsubscriber_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@

it 'stores the location in a thread local variable' do
subscriber.redirect_to(redirect_event)
expect(Thread.current[:lograge_location]).to eq('http://example.com')
expect(RequestStore.store[:lograge_location]).to eq('http://example.com')
end
end

Expand All @@ -91,7 +91,7 @@

it 'stores the parameters in a thread local variable' do
subscriber.unpermitted_parameters(unpermitted_parameters_event)
expect(Thread.current[:lograge_unpermitted_params]).to eq(%w(foo bar))
expect(RequestStore.store[:lograge_unpermitted_params]).to eq(%w(foo bar))
end
end

Expand Down Expand Up @@ -167,7 +167,7 @@

context 'with a redirect' do
before do
Thread.current[:lograge_location] = 'http://www.example.com'
RequestStore.store[:lograge_location] = 'http://www.example.com'
end

it 'adds the location to the log line' do
Expand All @@ -177,7 +177,7 @@

it 'removes the thread local variable' do
subscriber.process_action(event)
expect(Thread.current[:lograge_location]).to be_nil
expect(RequestStore.store[:lograge_location]).to be_nil
end
end

Expand All @@ -188,7 +188,7 @@

context 'with unpermitted_parameters' do
before do
Thread.current[:lograge_unpermitted_params] = %w(florb blarf)
RequestStore.store[:lograge_unpermitted_params] = %w(florb blarf)
end

it 'adds the unpermitted_params to the log line' do
Expand All @@ -198,7 +198,7 @@

it 'removes the thread local variable' do
subscriber.process_action(event)
expect(Thread.current[:lograge_unpermitted_params]).to be_nil
expect(RequestStore.store[:lograge_unpermitted_params]).to be_nil
end
end

Expand Down

0 comments on commit b6ed2a8

Please sign in to comment.