Mailinglist Archive: yast-devel (77 mails)

< Previous Next >
[yast-devel] Re: [yast-commit] <rest-service> backgroud_patches_bnc550934 : patches and status - moved the shared background code
  • From: Josef Reidinger <jreidinger@xxxxxxx>
  • Date: Wed, 16 Dec 2009 09:23:16 +0100
  • Message-id: <200912160923.16755.jreidinger@xxxxxxx>
Ladislav Slezak write:
ref: refs/heads/backgroud_patches_bnc550934
commit fe2894a7088810c100f1eac4a071a6911e8193ba
Author: Ladislav Slezak <lslezak@xxxxxxxxxx>
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@xxxxxxxxxxxx
For additional commands, e-mail: yast-devel+help@xxxxxxxxxxxx

< Previous Next >