Forum OpenACS Improvement Proposals (TIPs): Re: Display errors that occurred while sourcing a file in APM package-reload page.

> The patch already only creates one mutex...

hmm, i thought your concern was that multiple mutexes are created, when packages/acs-bootstrap-installer/tcl/30-apm-load-procs.tcl is sourced multiple times. To avoid this, the correct approach is as noted above.

apm_source is used from all connection threads to pick up the changed files. I would think that in acs-admin/www/apm/version-reload one is not interested about what happens in various threads but just in the errors of the reloaded files. Wouldn't it be much easier the get this information directly in acs-admin/www/apm/version-reload? One could provide e.g. an optional variable name to apm_source with the effect that in case of an error this variable is set. One could use something in the lines of

if {[apm_source $file err] == 0]} {... do something with $err ...}
in acs-admin/www/apm/version-reload.

This path looks more straightforward to me than going into more and more locking.

Yes, we could simplify the error-message accumulation if we add an optional variableName parameter to apm_load_any_changed_libraries which gets passed down to apm_source (and db_qd_load_query_file) as well. The code in those procs could then accumulate the error messages without any locking:


Index: acs-bootstrap-installer/tcl/30-apm-load-procs.tcl
===================================================================
--- acs-bootstrap-installer/tcl/30-apm-load-procs.tcl
+++ acs-bootstrap-installer/tcl/30-apm-load-procs.tcl
@@ -377,25 +379,33 @@
     return [expr ![file exists "${package_path}/sql"] || [file exists "${package_path}/sql/[db_type]"]]
 }
 
-ad_proc -private apm_source { __file } {
+ad_proc -private apm_source { __file {error_set_name ""}} {
     Sources $__file in a clean environment, returning 1 if successful or 0 if not.
     Records that the file has been sourced and stores its mtime in the nsv array
     apm_library_mtime
 } {
+    if {$error_set_name ne ""} {
+	upvar $error_set_name errors
+    } else {
+	array set errors [list]
+    }
+
     if { ![file exists $__file] } {
		ns_log "Error" "Unable to source $__file: file does not exist."
 	return 0
     }
 
+    set r_file [ad_make_relative_path $__file]
+
     # Actually do the source.
     if { [catch { source $__file }] } {
 	global errorInfo
		ns_log "Error" "Error sourcing $__file:\n$errorInfo"
+	set errors($r_file) $errorInfo
 	return 0
     }
 
-    nsv_set apm_library_mtime [ad_make_relative_path $__file] [file mtime $__file]    
+    nsv_set apm_library_mtime $r_file [file mtime $__file]

     return 1
 }



Index: acs-bootstrap-installer/tcl/40-db-query-dispatcher-procs.tcl
===================================================================
--- acs-bootstrap-installer/tcl/40-db-query-dispatcher-procs.tcl
+++ acs-bootstrap-installer/tcl/40-db-query-dispatcher-procs.tcl
@@ -217,14 +217,24 @@
 #
 ################################################
 
-ad_proc -public db_qd_load_query_file {file_path} {
+ad_proc -public db_qd_load_query_file {file_path {error_set_name ""}} {
     A procedure that is called from the outside world (APM) 
     to load a particular file
 } { 
+    if {$error_set_name ne ""} {
+	upvar $error_set_name errors
+    } else {
+	array set errors [list]
+    }
+
     if { [catch {db_qd_internal_load_cache $file_path} errmsg] } {
         global errorInfo
         ns_log Error "Error parsing queryfile $file_path:\n\n$errmsg\n\n$errorInfo"
+	set r_file [ad_make_relative_path $file_path]
+	set errors($r_file) $errorInfo
     }
 }



Index: acs-tcl/tcl/apm-procs.tcl
===================================================================
--- acs-tcl/tcl/apm-procs.tcl
+++ acs-tcl/tcl/apm-procs.tcl
@@ -739,7 +739,7 @@
     " -default "all" -bind [list db_type $db_type]]]
 }
 
-ad_proc -public apm_load_any_changed_libraries {} {
+ad_proc -public apm_load_any_changed_libraries {{error_set_name {}}} {
     
     In the running interpreter, reloads files marked for reload by
     apm_mark_version_for_reload. If any watches are set, examines watched
@@ -748,6 +748,10 @@
     before any filters or registered procedures are applied).
 
 } {
+    if {$error_set_name ne ""} {
+	upvar $error_set_name errors
+    } else {
+	array set errors [list]
+    }
+
     # Determine the current reload level in this interpreter by calling
     # apm_reload_level_in_this_interpreter. If this fails, we define the reload level to be
     # zero.
@@ -807,12 +811,12 @@
                         # Make sure this is not a -init.tcl file as those should only be sourced on server startup
                         if { ![regexp {\-init\.tcl$} $file_path] } {
                             ns_log Notice "apm_load_any_changed_libraries: Reloading $file..."
-                            apm_source $file_path
+                            apm_source $file_path errors
                         }
                     }
                     .xql { 
                         ns_log Notice "apm_load_any_changed_libraries: Reloading $file..."
-                        db_qd_load_query_file $file_path
+                        db_qd_load_query_file $file_path errors
                     }
                     default {
                         ns_log Notice "apm_load_any_changed_libraries: File $file_path has unknown extension. Not reloading."



Index: acs-admin/www/apm/version-reload.tcl
===================================================================
--- acs-admin/www/apm/version-reload.tcl
+++ acs-admin/www/apm/version-reload.tcl
@@ -23,7 +23,13 @@
 if { [llength $files] == 0 } {
     append body "There are no changed files to reload in this package.<p>"
 } else {
-    append body "Marked the following file[ad_decode [llength $files] 1 "" "s"] for reloading:<ul>\n"
+    append body "Marked the following file[ad_decode [llength $files] 1 "" "s"] for reloading:<ul id='files'>\n"
+
+    # Source all of the marked files using the current interpreter, accumulating
+    # errors into apm_package_load_errors
+    array set errors [list]
+    catch { apm_load_any_changed_libraries errors}
+
     foreach file $files {
 	append body "<li>$file"
 	if { [nsv_exists apm_reload_watch $file] } {
@@ -38,9 +44,29 @@
 	    append body " (<a href=\"file-watch?[export_vars { version_id { paths $local_path } }]\">watch this file</a>)"
             lappend files_to_watch $local_path
 	}
-	append body "\n"
+
+	if {[info exists errors($file)]} {
+	    append body "<dl class='error'><dt title='click to see details'>ERROR! (click for details)</dt><dd><code><pre>[ad_quotehtml $errors($file)]</pre></code></dd></dl>"
+	}
+	append body "</li>\n"
     }
     append body "</ul>\n"
+
+    set n_errors [array size errors]
+    if {$n_errors > 0} {
+	if {$n_errors != 1} {
+	    set exist_n_error_files "were $n_errors files"
+	} else {
+	    set exist_n_error_files "was $n_errors file"
+	}
+	append body "
+		<p><strong style='color:red;font-size:112.5%;'>There
+		$exist_n_error_files with errors that prevented complete
+		reloading</strong>.  Fix the problem, then reload the
+		package again to finish the reload.
+		</p>
+	"
+    }
 }



Index: acs-admin/www/apm/version-reload.adp
===================================================================
--- acs-admin/www/apm/version-reload.adp
+++ acs-admin/www/apm/version-reload.adp
@@ -2,4 +2,19 @@
 <property name="title">@page_title;noquote@</property>
 <property name="context">@context;noquote@</property>
 
+<style type="text/css">
+  dl.error, dl.error dt { display:inline; color:red; }
+  dl.error dt { font-weight:bold; }
+  dl.error    { margin-left:1em; cursor:pointer; }
+  dl.error dd { display:none; }
+</style>
 @body;noquote@
+<script type="text/javascript">
+  <% template::head::add_javascript -src "//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js" %<
+  $('#files dd').click(function () {
+	$(this).slideToggle();
+  });
+  $('#files dt').click(function () {
+	$(this.nextElementSibling).slideToggle();
+  });
+</script>

This is ok if the only place we want to see error messages is in the packages/acs-admin/www/apm/version-reload page. If it would also be useful to show an error-indicator on the packages/acs-admin/www/apm/index page (for example, if a server was started with syntax errors in some of its package files), then we would want to stick with the global shared variable approach. I've implemented this in our sites using 10 more lines of code in the apm/index page.

My last concern with the passed error-accumulator is that some of the connection threads encounter errors during apm_sourceing which the other connection threads do not. This has happened on some of our sites where a reload will fix a bug on some connection threads, but repeated reloads of a page that uses the new code reveals the bug still is produced by other connection threads. In the past, we have just restarted AOLserver to clear these up. With an error message output, we may not be able to identify the thread, but we could identify the bug that prevented a thread from loading the file and arrange for another reload to occur so the stuck thread gets the fresh code too.

That patch has some errors. One is due to hand-quoting the HTML embedded in the patch (see this bug):
+  <% template::head::add_javascript -src "//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js" %<
…should be:
+  <% template::head::add_javascript -src "//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js" %>

Not sure if any more of those remain, so caveat emptor.

Also, it occurs to me that, as was expected in the 1st patch in the thread, apm_load_any_changed_libraries will load new code without any attention to package boundaries, so set errors($r_file) $errorInfo needs to be lappend errors($package_key) $r_file $errorInfo with suitable code for getting the package_key from the full file name. This means a number of changes to the patch chunk for acs-admin/www/apm/version-reload.tcl as well.

The patch (fixes in green)



Index: acs-bootstrap-installer/tcl/30-apm-load-procs.tcl
===================================================================
--- acs-bootstrap-installer/tcl/30-apm-load-procs.tcl
+++ acs-bootstrap-installer/tcl/30-apm-load-procs.tcl
@@ -377,25 +379,33 @@
     return [expr ![file exists "${package_path}/sql"] || [file exists "${package_path}/sql/[db_type]"]]
 }
 
-ad_proc -private apm_source { __file } {
+ad_proc -private apm_source { __file {error_set_name ""}} {
     Sources $__file in a clean environment, returning 1 if successful or 0 if not.
     Records that the file has been sourced and stores its mtime in the nsv array
     apm_library_mtime
 } {
+    if {$error_set_name ne ""} {
+	upvar $error_set_name errors
+    } else {
+	array set errors [list]
+    }
+
     if { ![file exists $__file] } {
		ns_log "Error" "Unable to source $__file: file does not exist."
 	return 0
     }
 
+    set r_file [ad_make_relative_path $__file]
+
     # Actually do the source.
     if { [catch { source $__file }] } {
 	global errorInfo
		ns_log "Error" "Error sourcing $__file:\n$errorInfo"
+	set package_key ""
+	regexp {/packages/([^/]+)/} $__file -> package_key
+	lappend errors($package_key) $r_file $errorInfo
 	return 0
     }
 
-    nsv_set apm_library_mtime [ad_make_relative_path $__file] [file mtime $__file]    
+    nsv_set apm_library_mtime $r_file [file mtime $__file]

     return 1
 }



Index: acs-bootstrap-installer/tcl/40-db-query-dispatcher-procs.tcl
===================================================================
--- acs-bootstrap-installer/tcl/40-db-query-dispatcher-procs.tcl
+++ acs-bootstrap-installer/tcl/40-db-query-dispatcher-procs.tcl
@@ -217,14 +217,24 @@
 #
 ################################################
 
-ad_proc -public db_qd_load_query_file {file_path} {
+ad_proc -public db_qd_load_query_file {file_path {error_set_name ""}} {
     A procedure that is called from the outside world (APM) 
     to load a particular file
 } { 
+    if {$error_set_name ne ""} {
+	upvar $error_set_name errors
+    } else {
+	array set errors [list]
+    }
+
     if { [catch {db_qd_internal_load_cache $file_path} errmsg] } {
         global errorInfo
         ns_log Error "Error parsing queryfile $file_path:\n\n$errmsg\n\n$errorInfo"
+	set r_file [ad_make_relative_path $file_path]
+	set package_key ""
+	regexp {/packages/([^/]+)/} $file_path -> package_key
+	lappend errors($package_key) $r_file $errorInfo
     }
 }



Index: acs-tcl/tcl/apm-procs.tcl
===================================================================
--- acs-tcl/tcl/apm-procs.tcl
+++ acs-tcl/tcl/apm-procs.tcl
@@ -739,7 +739,7 @@
     " -default "all" -bind [list db_type $db_type]]]
 }
 
-ad_proc -public apm_load_any_changed_libraries {} {
+ad_proc -public apm_load_any_changed_libraries {{error_set_name {}}} {
     
     In the running interpreter, reloads files marked for reload by
     apm_mark_version_for_reload. If any watches are set, examines watched
@@ -748,6 +748,10 @@
     before any filters or registered procedures are applied).
 
 } {
+    if {$error_set_name ne ""} {
+	upvar $error_set_name errors
+    } else {
+	array set errors [list]
+    }
+
     # Determine the current reload level in this interpreter by calling
     # apm_reload_level_in_this_interpreter. If this fails, we define the reload level to be
     # zero.
@@ -807,12 +811,12 @@
                         # Make sure this is not a -init.tcl file as those should only be sourced on server startup
                         if { ![regexp {\-init\.tcl$} $file_path] } {
                             ns_log Notice "apm_load_any_changed_libraries: Reloading $file..."
-                            apm_source $file_path
+                            apm_source $file_path errors
                         }
                     }
                     .xql { 
                         ns_log Notice "apm_load_any_changed_libraries: Reloading $file..."
-                        db_qd_load_query_file $file_path
+                        db_qd_load_query_file $file_path errors
                     }
                     default {
                         ns_log Notice "apm_load_any_changed_libraries: File $file_path has unknown extension. Not reloading."



Index: acs-admin/www/apm/version-reload.tcl
===================================================================
--- acs-admin/www/apm/version-reload.tcl
+++ acs-admin/www/apm/version-reload.tcl
@@ -23,7 +23,13 @@
 if { [llength $files] == 0 } {
     append body "There are no changed files to reload in this package.<p>"
 } else {
-    append body "Marked the following file[ad_decode [llength $files] 1 "" "s"] for reloading:<ul>\n"
+    append body "Marked the following file[ad_decode [llength $files] 1 "" "s"] for reloading:<ul id='files'>\n"
+
+    # Source all of the marked files using the current interpreter, accumulating
+    # errors into apm_package_load_errors
+    array set errors [list]
+    catch { apm_load_any_changed_libraries errors}
+
+    if {[info exists errors($package_key)]} {
+ 	array set package_errors $errors($package_key)
+    } else {
+ 	array set package_errors [list]
+    }
+
     foreach file $files {
 	append body "<li>$file"
 	if { [nsv_exists apm_reload_watch $file] } {
@@ -38,9 +44,29 @@
 	    append body " (<a href=\"file-watch?[export_vars { version_id { paths $local_path } }]\">watch this file</a>)"
             lappend files_to_watch $local_path
 	}
-	append body "\n"
+
+	if {[info exists package_errors($file)]} {
+	    append body "<dl class='error'><dt title='click to see details'>ERROR! (click for details)</dt><dd><code><pre>[ad_quotehtml $package_errors($file)]</pre></code></dd></dl>"
+	}
+	append body "</li>\n"
     }
     append body "</ul>\n"
+
+    set n_errors [array size package_errors]
+    if {$n_errors > 0} {
+	if {$n_errors != 1} {
+	    set exist_n_error_files "were $n_errors files"
+	} else {
+	    set exist_n_error_files "was $n_errors file"
+	}
+	append body "
+		<p><strong style='color:red;font-size:112.5%;'>There
+		$exist_n_error_files with errors that prevented complete
+		reloading</strong>.  Fix the problem, then reload the
+		package again to finish the reload.
+		</p>
+	"
+    }
 }



Index: acs-admin/www/apm/version-reload.adp
===================================================================
--- acs-admin/www/apm/version-reload.adp
+++ acs-admin/www/apm/version-reload.adp
@@ -2,4 +2,19 @@
 <property name="title">@page_title;noquote@</property>
 <property name="context">@context;noquote@</property>
 
+<style type="text/css">
+  dl.error, dl.error dt { display:inline; color:red; }
+  dl.error dt { font-weight:bold; }
+  dl.error    { margin-left:1em; cursor:pointer; }
+  dl.error dd { display:none; }
+</style>
 @body;noquote@
+<script type="text/javascript">
+  <% template::head::add_javascript -src "//ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js" %>
+  $('#files dd').click(function () {
+	$(this).slideToggle();
+  });
+  $('#files dt').click(function () {
+	$(this.nextElementSibling).slideToggle();
+  });
+</script>