Ladislav Slezak write:

ref: refs/heads/backgroud_patches_bnc550934 commit fe2894a7088810c100f1eac4a071a6911e8193ba Author: Ladislav Slezak 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] + return [ get_progress(what) ] 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, :limits

- # 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