Ladislav Slezak write:
> ref: refs/heads/backgroud_patches_bnc550934
> commit fe2894a7088810c100f1eac4a071a6911e8193ba
> Author: Ladislav Slezak <lslezak(a)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]
> + 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(a)opensuse.org
For additional commands, e-mail: yast-devel+help(a)opensuse.org
