Ladislav Slezak write:
ref: refs/heads/backgroud_patches_bnc550934 commit fe2894a7088810c100f1eac4a071a6911e8193ba Author: Ladislav Slezak <lslezak@novell.com> Date: Tue Dec 15 20:13:59 2009 +0100
patches and status - moved the shared background code to BackgroundManager module (for thread safe acess and better readability)
plugins/patches/app/models/patch.rb | 53 +++++++++------------- plugins/status/lib/status.rb | 78 ++++++++++++++------------------ webservice/lib/background_manager.rb | 80 ++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 76 deletions(-)
diff --git a/plugins/patches/app/models/patch.rb b/plugins/patches/app/models/patch.rb index dd861de..a3608c6 100644 --- a/plugins/patches/app/models/patch.rb +++ b/plugins/patches/app/models/patch.rb @@ -3,14 +3,10 @@ require 'resolvable' # Model for patches available via package kit class Patch < Resolvable
- # class variables - they keep the information between requests
- #
- # currently running patch requests in background (current states)
- @@running = Hash.new
- # finished requests (actual results)
- @@done = Hash.new
- # a mutex which guards access to the shared class variables above
- @@mutex = Mutex.new
# extend adds the module methods as class methods
# (used instead of include because background processes
# are used in class methods here)
extend BackgroundManager
def to_xml( options = {} ) super :patch_update, options @@ -35,7 +31,7 @@ class Patch < Resolvable return patch_updates end
- def self.subprocess_find(what, bs)
- def self.subprocess_find(what)
Hi, few notes. I think that name of variable is not good and quite confusing. Especially when you cal it below as e.g. "get_process what". I suggest to use variable name e.g. process_name or process_id or similar.
# open subprocess subproc = IO.popen(subprocess_command)
@@ -55,7 +51,8 @@ class Patch < Resolvable result = received['patches'] elsif received.has_key? 'background_status' s = received['background_status']
@@mutex.synchronize do
update_progress what do |bs| bs.status = s['status'] bs.progress = s['progress'] bs.subprogress = s['subprogress']
@@ -86,42 +83,34 @@ class Patch < Resolvable
# background reading doesn't work correctly if class reloading is active # (static class members are lost between requests)
- if background && !Rails.configuration.cache_classes
if background && !background_enabled? Rails.logger.info "Class reloading is active, cannot use background thread (set config.cache_classes = true)" background = false end
if background
# background status
bs = nil
@@mutex.synchronize do
if @@done.has_key?(what)
Rails.logger.debug "Request #{what} is done"
return @@done.delete(what)
end
running = @@running[what]
if running
Rails.logger.debug "Request #{what} is already running: #{running.inspect}"
return [running]
end
if process_finished? what
Rails.logger.debug "Request #{what} is done"
return get_value what
end
bs = BackgroundStatus.new
@@running[what] = bs
running = get_progress what
if running
Rails.logger.debug "Request #{what} is already running: #{running.inspect}"
return [running] end
add_process what
Rails.logger.info "Starting background thread for reading patches..." # run the patch query in a separate thread Thread.new do
res = subprocess_find(what, bs)
res = subprocess_find what Rails.logger.info "*** Patches thread: Found #{res.size} applicable patches"
@@mutex.synchronize do
@@running.delete(what)
@@done[what] = res
end
finish_process(what, res) end
return [bs]
else return do_find(what) end diff --git a/plugins/status/lib/status.rb b/plugins/status/lib/status.rb index 1bcdabe..cad3412 100644 --- a/plugins/status/lib/status.rb +++ b/plugins/status/lib/status.rb @@ -10,12 +10,7 @@ class Status :metrics, :limitsreturn [ get_progress(what) ]
- # currently running status requests in background (current states)
- @@running = Hash.new
- # finished requests (actual results)
- @@done = Hash.new
- # a mutex which guards access to the shared class variables above
- @@mutex = Mutex.new
include BackgroundManager
def to_xml(options = {}) if @data.class == Hash && @data.size == 1 && @data[:progress].class == BackgroundStatus @@ -152,51 +147,48 @@ class Status def draw_graph(heigth=nil, width=nil) end
def create_unique_key(start, stop, data)
start.to_i.to_s + '_' + stop.to_i.to_s + '_' + data.to_s
end
# this is a wrapper to collect_data def collect_data(start=nil, stop=nil, data = %w{cpu memory disk}, background = false) # background reading doesn't work correctly if class reloading is active # (static class members are lost between requests)
- if background && !Rails.configuration.cache_classes
if background && !background_enabled? Rails.logger.info "Class reloading is active, cannot use background thread (set config.cache_classes = true)" background = false end
if background
# background status
bs = nil
key = start.to_i.to_s + '_' + stop.to_i.to_s + '_' + data.to_s
@@mutex.synchronize do
if @@done.has_key?(key)
Rails.logger.debug "Request #{key} is done"
@data = @@done.delete key
@data.delete :progress
return @data
end
key = create_unique_key(start, stop, data)
running = @@running[key]
if running
Rails.logger.debug "Request #{key} is already running: #{running.inspect}"
@data = {:progress => running}
return @data
end
if process_finished? key
Rails.logger.debug "Request #{key} is done"
@data = get_value key
@data.delete :progress
return @data
end
bs = BackgroundStatus.new
@@running[key] = bs
running = get_progress key
if running
Rails.logger.debug "Request #{key} is already running: #{running.inspect}"
@data = {:progress => running}
return @data end
add_process(key) # read the status in a separate thread Thread.new do Rails.logger.info '*** Background thread for reading status started...'
res = collect_status_data(start, stop, data, bs)
@@mutex.synchronize do
@@running.delete(key)
@@done[key] = res
end
res = collect_status_data(start, stop, data, true)
finish_process(key, res) Rails.logger.info '*** Finishing background status thread' end # return current progress
@data = {:progress => bs}
@data = {:progress => get_progress(key)} return @data
else collect_status_data(start, stop, data) @@ -204,8 +196,8 @@ class Status end
# creates several metrics for a defined period
- def collect_status_data(start=nil, stop=nil, data = %w{cpu memory disk}, progress=nil)
- def collect_status_data(start=nil, stop=nil, data = %w{cpu memory disk}, progress=false)
- key = create_unique_key(start, stop, data) if progress metrics = available_metrics #puts metrics.inspect result = Hash.new @@ -213,16 +205,14 @@ class Status current_step = 0 if progress case data when nil, "all", "All" # all metrics
if progress
total_steps = metrics.size
end
total_steps = metrics.size if progress metrics.each_pair do |m, n| if progress
@@mutex.synchronize do
progress.status = m.to_s
progress.progress = 100 * current_step / total_steps
Rails.logger.debug "*** Reading status: #{progress.status}: #{progress.progress}%"
update_progress key do |stat|
stat.status = m.to_s
stat.progress = 100 * current_step / total_steps
Rails.logger.debug "*** Reading status: #{stat.status}: #{stat.progress}%" end end
@@ -238,10 +228,10 @@ class Status total_steps = data.size if progress data.each do |d| if progress
@@mutex.synchronize do
progress.status = d.to_s
progress.progress = 100 * current_step / total_steps
Rails.logger.debug "*** Reading status: #{progress.status}: #{progress.progress}%"
update_progress key do |stat|
stat.status = d.to_s
stat.progress = 100 * current_step / total_steps
Rails.logger.debug "*** Reading status: #{stat.status}: #{stat.progress}%" end end
diff --git a/webservice/lib/background_manager.rb b/webservice/lib/background_manager.rb new file mode 100644 index 0000000..722d078 --- /dev/null +++ b/webservice/lib/background_manager.rb @@ -0,0 +1,80 @@ + +# this is a helper module for managing background processes with progress +# for long running tasks + +module BackgroundManager
- # class variables - they keep the information between requests
- # DO NOT USE THEM DIRECTLY, USE THE THREAD SAFE METHODS BELOW!
When I see what provides this module and how it heavily use class variables I think that something is bad with design of this module (too many @@, no instance variable, instance methods doesn't use instance data (because noone is present)). When I think about this module I see this solution as the best one: Use singleton class which manages background process and have hash of running, done and mutex as instance variables
- #
- # currently running requests in background (current states)
- @@running = Hash.new
- # finished requests (actual results)
- @@done = Hash.new
- # a mutex which guards access to the shared class variables above
- @@mutex = Mutex.new
- # define a new background process
- # id is unique ID
- def add_process(id)
- @@mutex.synchronize do
@@running[id] = BackgroundStatus.new unless @@running.has_key?(id)
- end
- end
- # is the process running?
- def process_running?(id)
- @@mutex.synchronize do
return @@running.has_key? id
This doesn't work. You cannot return from from block ( I already study this problem when catch problem in validates_each ). Because it return data in block which return statement could be ignored ( because synchronize do lock; yield,unlock so even if this work it return value from unlock - if the most case it is expected value but can be nil see below). just try it in irb: m.synchronize { return 'a' } LocalJumpError: unexpected return from (irb):5 from (irb):5:in `synchronize' from (irb):5 from :0 so use instead variable ret = false @@mutex.synchronize do ret = @@running.has_key? id end return ret # I personally prefer explicit return even if it is not needed in ruby This lead me to question - is this code tested? It raise exception each time, so it is strange for that it is not discovered (I found my problem with validation in tests).
- end
- end
- # is the process finished?
- def process_finished?(id)
- @@mutex.synchronize do
return @@done.has_key? id
^^^ same as above
- end
- end
- # remove the progress status and remember the real final value
- def finish_process(id, value)
- @@mutex.synchronize do
@@running.delete(id)
@@done[id] = value
- end
- end
- # get the current progress
- # returns a copy, use update_progress() for updating the progress
- def get_progress(id)
- @@mutex.synchronize do
ret = @@running[id]
ret.nil? ? nil : ret.dup
^^^ This return excepted value if you not make critical mistake ( because it return nil, if you unlock mutex on which you invoke synchronize method. So I recommend checking return value of synchronize to detect locking problem (but you must change inside block to not return nil in any case). so something like : ret = nil mret = @@mutex.synchronize do ret = @@running[id] ret = ret.dup unless ret.nil? "OK" done raise "Someone unlock mutex during executing synchronize" unless mret return ret working in threads and locking need special care and study how it really work, otherwise some really ugly problem can raise.
end
end
# get the final value, the value is removed from the internal structure
def get_value(id)
@@mutex.synchronize do
return @@done.delete id
^^^ same as above
end
end
# update the progress
def update_progress(id, &block)
@@mutex.synchronize do
bs = @@running[id]
yield bs if bs && block_given?
end
end
# is background processing possible?
# if cache_classes is disabled it is not possible
# because all classes are reloaded between requests
# and the static attributes keeping the progress are lost
def background_enabled?
return Rails.configuration.cache_classes
end
+end
When I check this commit I also found some not bad practice in BackgroundStatus: attr_reader :status def status=(status) if @status != status @status = status trigger_callback end end I don't like it, because it is confusing use same name for parameter and for attribute. There is confusing if status is status reader or argument (of course if you know how in deep ruby work ,then parameter as local variable overwrite instance method, but I find it confusing. and little hints: def trigger_callback if @callback @callback.call end end you could write it as def trigger_callback @callback.try(:call) end (or if you don't want use rails object extension you can still write it as oneliner) you don't provide to_json method which could be problem if we switch frontend to json. There you can benefit from serialization module which I write for BaseModel. So instead of whole to_xml method you can get same result (and as bonus also to_json and from_xml and from_json method) if you use this code: (attr_serialized is needed to avoid callback instance serialization) include BaseModel::Serialization attr_serialized :status, :progress, :subprogress Note - all code is code from head, so no guaranty that it works out of box, but it should work with little fixes. Josef -- Josef Reidinger YaST team maintainer of perl-Bootloader, YaST2-Repair, webyast (language,time,basesystem,ntp) -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org For additional commands, e-mail: yast-devel+help@opensuse.org