Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brpoplpush redis script #434

Merged
merged 9 commits into from
Oct 14, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions .reek.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ detectors:
exclude:
- SidekiqUniqueJobs::DeleteOrphan
- SidekiqUniqueJobs::Redis
- SidekiqUniqueJobs::Script
DuplicateMethodCall:
exclude:
- Sidekiq#self.use_options
Expand All @@ -49,13 +48,9 @@ detectors:
- SidekiqUniqueJobs::Logging#debug_item
- SidekiqUniqueJobs::NotUniqueWorker#initialize
- SidekiqUniqueJobs::OnConflict::Reject#push_to_deadset
- SidekiqUniqueJobs::ScriptError#generate_error_context
- SidekiqUniqueJobs::Web::Helpers#cparams
InstanceVariableAssumption:
exclude:
- SidekiqUniqueJobs::Script::Template
IrresponsibleModule:
enabled: false
enabled: true
LongParameterList:
enabled: true
exclude:
Expand All @@ -64,8 +59,6 @@ detectors:
- SidekiqUniqueJobs::Changelog#add
- SidekiqUniqueJobs::Middleware#call
- SidekiqUniqueJobs::Redis
- SidekiqUniqueJobs::Script#call
- SidekiqUniqueJobs::Script#execute
- SidekiqUniqueJobs::Script::Caller#call_script
- SidekiqUniqueJobs::Script::Caller#do_call
ManualDispatch:
Expand Down Expand Up @@ -124,11 +117,8 @@ detectors:
- SidekiqUniqueJobs::Middleware#call
- SidekiqUniqueJobs::Middleware#self.configure_server
- SidekiqUniqueJobs::Profiler#self.stop
- SidekiqUniqueJobs::Script#call
- SidekiqUniqueJobs::Script#handle_error
- SidekiqUniqueJobs::Script::Caller#call_script
- SidekiqUniqueJobs::Script::Caller#extract_args
- SidekiqUniqueJobs::ScriptError#generate_error_context
- SidekiqUniqueJobs::TimeCalculator#lock_ttl
- SidekiqUniqueJobs::UniqueArgs#filtered_args
- SidekiqUniqueJobs::UpgradeLocks#call
Expand Down
2 changes: 0 additions & 2 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,6 @@ Style/Documentation:
- "lib/sidekiq_unique_jobs/core_ext.rb"
- "lib/sidekiq_unique_jobs/sidekiq_unique_ext.rb"
- "lib/sidekiq_unique_jobs/web/**/*"
- "lib/sidekiq_unique_jobs/on_conflict.rb"
- "lib/sidekiq_unique_jobs/timeout.rb"

Style/FrozenStringLiteralComment:
Enabled: true
Expand Down
4 changes: 2 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,6 @@ matrix:
include:
- rvm: jruby-9.2.8.0
gemfile: gemfiles/sidekiq_6.0.gemfile
- rvm: 2.6.5
gemfile: gemfiles/sidekiq_develop.gemfile
- rvm: 2.6.5
gemfile: gemfiles/sidekiq_6.0.gemfile
env: COV=true
Expand All @@ -58,6 +56,8 @@ gemfile:
- gemfiles/sidekiq_5.0.gemfile
- gemfiles/sidekiq_5.1.gemfile
- gemfiles/sidekiq_5.2.gemfile
- gemfiles/sidekiq_6.0.gemfile
- gemfiles/sidekiq_develop.gemfile

notifications:
email:
Expand Down
2 changes: 1 addition & 1 deletion lib/sidekiq_unique_jobs.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require "brpoplpush/redis_script"
require "concurrent/future"
require "concurrent/promises"
require "concurrent/timer_task"
Expand All @@ -25,7 +26,6 @@
require "sidekiq_unique_jobs/exceptions"
require "sidekiq_unique_jobs/script"
require "sidekiq_unique_jobs/script/caller"
require "sidekiq_unique_jobs/script/template"
require "sidekiq_unique_jobs/json"
require "sidekiq_unique_jobs/normalizer"
require "sidekiq_unique_jobs/job"
Expand Down
83 changes: 0 additions & 83 deletions lib/sidekiq_unique_jobs/exceptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,89 +82,6 @@ def initialize(options: {})
end
end

# Error raised from {OnConflict::Raise}
#
# @author Mikael Henriksson <mikael@zoolutions.se>
class ScriptError < UniqueJobsError
# Reformats errors raised by redis representing failures while executing
# a lua script. The default errors have confusing messages and backtraces,
# and a type of +RuntimeError+. This class improves the message and
# modifies the backtrace to include the lua script itself in a reasonable
# way.

PATTERN = /ERR Error (compiling|running) script \(.*?\): .*?:(\d+): (.*)/.freeze
LIB_PATH = File.expand_path("..", __dir__)
CONTEXT_LINE_NUMBER = 3

attr_reader :error, :file, :content

# Is this error one that should be reformatted?
#
# @param error [StandardError] the original error raised by redis
# @return [Boolean] is this an error that should be reformatted?
def self.intercepts?(error)
error.message =~ PATTERN
end

# Initialize a new {ScriptError} from an existing redis error, adjusting
# the message and backtrace in the process.
#
# @param error [StandardError] the original error raised by redis
# @param file [Pathname] full path to the lua file the error ocurred in
# @param content [String] lua file content the error ocurred in
# :nocov:
def initialize(error, file, content)
@error = error
@file = file
@content = content
@backtrace = @error.backtrace

@error.message.match(PATTERN) do |regexp_match|
line_number = regexp_match[2].to_i
message = regexp_match[3]
error_context = generate_error_context(content, line_number)

super("#{message}\n\n#{error_context}\n\n")
set_backtrace(generate_backtrace(file, line_number))
end
end

private

# :nocov:
def generate_error_context(content, line_number)
lines = content.lines.to_a
beginning_line_number = [1, line_number - CONTEXT_LINE_NUMBER].max
ending_line_number = [lines.count, line_number + CONTEXT_LINE_NUMBER].min
line_number_width = ending_line_number.to_s.length

(beginning_line_number..ending_line_number).map do |number|
indicator = (number == line_number) ? "=>" : " "
formatted_number = format("%#{line_number_width}d", number)
" #{indicator} #{formatted_number}: #{lines[number - 1]}"
end.join.chomp
end

# :nocov:
def generate_backtrace(file, line_number)
pre_gem = backtrace_before_entering_gem(@backtrace)
index_of_first_unique_jobs_line = (@backtrace.size - pre_gem.size - 1)
pre_gem.unshift(@backtrace[index_of_first_unique_jobs_line])
pre_gem.unshift("#{file}:#{line_number}")
pre_gem
end

# :nocov:
def backtrace_before_entering_gem(backtrace)
backtrace.reverse.take_while { |line| !line_from_gem(line) }.reverse
end

# :nocov:
def line_from_gem(line)
line.split(":").first.include?(LIB_PATH)
end
end

# Error raised from {OptionsWithFallback#lock_class}
#
# @author Mikael Henriksson <mikael@zoolutions.se>
Expand Down
5 changes: 5 additions & 0 deletions lib/sidekiq_unique_jobs/lock/until_expired.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

module SidekiqUniqueJobs
class Lock
#
# UntilExpired locks until the job expires
#
# @author Mikael Henriksson <mikael@zoolutions.se>
#
class UntilExpired < UntilExecuted
end
end
Expand Down
4 changes: 2 additions & 2 deletions lib/sidekiq_unique_jobs/orphans/manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class Manager
#
def self.start
with_logging_context do
logger.info("Starting Reaper")
log_info("Starting Reaper")
task.add_observer(Observer.new)
task.execute
task
Expand All @@ -34,7 +34,7 @@ def self.start
#
def self.stop
with_logging_context do
logger.info("Stopping Reaper")
log_info("Stopping Reaper")
task.shutdown
end
end
Expand Down
125 changes: 4 additions & 121 deletions lib/sidekiq_unique_jobs/script.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,128 +5,11 @@ module SidekiqUniqueJobs
#
# @author Mikael Henriksson <mikael@zoolutions.se>
module Script
LUA_PATHNAME ||= Pathname.new(__FILE__).dirname.join("lua").freeze
SCRIPT_SHAS ||= Concurrent::Map.new
include Brpoplpush::RedisScript::DSL

extend SidekiqUniqueJobs::Connection
extend SidekiqUniqueJobs::Logging
extend SidekiqUniqueJobs::Timing

module_function

#
# Call a lua script with the provided file_name
#
# @note this method is recursive if we need to load a lua script
# that wasn't previously loaded.
#
# @param [Symbol] file_name the name of the lua script
# @param [Array<String>] keys script keys
# @param [Array<Object>] argv script arguments
# @param [Redis] conn the redis connection to use
#
# @return value from script
#
def call(file_name, conn, keys: [], argv: [])
result, elapsed = timed do
execute_script(file_name, conn, keys, argv)
end

log_debug("Executed #{file_name}.lua in #{elapsed}ms")
result
rescue ::Redis::CommandError => ex
handle_error(ex, file_name, conn) do
call(file_name, conn, keys: keys, argv: argv)
end
end

#
# Execute the script file
#
# @param [Symbol] file_name the name of the lua script
# @param [Redis] conn the redis connection to use
# @param [Array] keys the array of keys to pass to the script
# @param [Array] argv the array of arguments to pass to the script
#
# @return value from script (evalsha)
#
def execute_script(file_name, conn, keys, argv)
conn.evalsha(
script_sha(conn, file_name),
keys,
argv,
)
end

#
# Return sha of already loaded lua script or load it and return the sha
#
# @param [Sidekiq::RedisConnection] conn the redis connection
# @param [Symbol] file_name the name of the lua script
# @return [String] sha of the script file
#
# @return [String] the sha of the script
#
def script_sha(conn, file_name)
if (sha = SCRIPT_SHAS.get(file_name))
return sha
end

sha = conn.script(:load, script_source(file_name))
SCRIPT_SHAS.put(file_name, sha)
sha
end

#
# Handle errors to allow retrying errors that need retrying
#
# @param [Redis::CommandError] ex exception to handle
# @param [Symbol] file_name the name of the lua script
# @param [Redis] conn the redis connection to use
#
# @return [void]
#
# @yieldreturn [void] yields back to the caller when NOSCRIPT is raised
def handle_error(ex, file_name, conn) # rubocop:disable Metrics/MethodLength, Metrics/CyclomaticComplexity
case ex.message
when "NOSCRIPT No matching script. Please use EVAL."
SCRIPT_SHAS.delete(file_name)
return yield if block_given?
when "BUSY Redis is busy running a script. You can only call SCRIPT KILL or SHUTDOWN NOSAVE."
begin
conn.script(:kill)
return yield if block_given?
rescue ::Redis::CommandError => ex
log_warn(ex)
return yield if block_given?
end
end

raise unless ScriptError.intercepts?(ex)

raise ScriptError.new(ex, script_path(file_name).to_s, script_source(file_name))
end

#
# Reads the lua file from disk
#
# @param [Symbol] file_name the name of the lua script
#
# @return [String] the content of the lua file
#
def script_source(file_name)
Template.new(LUA_PATHNAME).render(script_path(file_name))
end

#
# Construct a Pathname to a lua script
#
# @param [Symbol] file_name the name of the lua script
#
# @return [Pathname] the full path to the gems lua script
#
def script_path(file_name)
LUA_PATHNAME.join("#{file_name}.lua")
configure do |config|
config.scripts_path = Pathname.new(__FILE__).dirname.join("lua")
config.logger = Sidekiq.logger # TODO: This becomes a little weird
end
end
end
2 changes: 1 addition & 1 deletion lib/sidekiq_unique_jobs/script/caller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def do_call(file_name, conn, keys, argv)
file_name,
redis_version,
])
Script.call(file_name, conn, keys: keys, argv: argv)
Script.execute(file_name, conn, keys: keys, argv: argv)
end

#
Expand Down
41 changes: 0 additions & 41 deletions lib/sidekiq_unique_jobs/script/template.rb

This file was deleted.

1 change: 1 addition & 0 deletions sidekiq-unique-jobs.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Gem::Specification.new do |spec|

spec.required_ruby_version = ">= 2.5.0"

spec.add_dependency "brpoplpush-redis_script", "> 0.0.0", "<= 2.0.0"
spec.add_dependency "concurrent-ruby", "~> 1.0", ">= 1.0.5"
spec.add_dependency "sidekiq", ">= 4.0", "< 7.0"
spec.add_dependency "thor", "~> 0"
Expand Down
Loading