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

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.