Bug 55004 - AfterEveryScript scripts should not treat abort_page as an error
Summary: AfterEveryScript scripts should not treat abort_page as an error
Status: RESOLVED FIXED
Alias: None
Product: Rivet
Classification: Unclassified
Component: Rivet Core Commands (show other bugs)
Version: trunk
Hardware: All All
: P2 normal
Target Milestone: mod_rivet
Assignee: Apache Rivet Mailing list account
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-22 10:46 UTC by Karl Lehenbauer
Modified: 2015-12-15 14:48 UTC (History)
2 users (show)



Attachments
sorry, wrong bug (8.04 KB, text/plain)
2013-07-02 08:40 UTC, Harald Oehlmann
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Karl Lehenbauer 2013-05-22 10:46:11 UTC
Right now if Rivet's configured with an AfterEveryScript and that script does an abort_page, it is treated as an error and it writes an emergency "<b>Rivet AfterEveryScript failed!</b>" and logs a traceback.

The check is performed near the end of Rivet_ExecuteAndCheck in mod_rivet.c

Before emitting the message an inquiry of Tcl's errorCode variable (or whatever C-level mechanism is available) to see if the source of the error is abort_page and in that case the error message should not be issued.
Comment 1 Karl Lehenbauer 2013-05-22 11:00:03 UTC
Since filing this I have been wondering if it is reasonable to invoke abort_page in an AfterEveryScript handler.  If we stopped doing that, we'd stop getting the error, with no changes to Rivet.

If Rivet is to be modified to accept abort_page as not an error in an AfterEveryScript, then there's code earlier in Rivet_ExecuteAndCheck that does exactly that.

It's complicated enough, though, that it should probably be factored into a routine that returns a 1/0 as to whether or not the Tcl error is RIVET ABORTPAGE.
Comment 2 Massimo Manghi 2013-05-22 20:37:06 UTC
Yes, abort_page was meant to interrupt execution in any scripts before AbortScript (the abort handler) and eventualy AfterEveryScript (also AfterScript can be interrupted by abort_page IIRC). Having abort_page interrupt the execution flow after AbortPage doesn't fit the design because AfterEveryScript was meant to run as very last execution segment. 

As to the routine returning a boolean condition I'm not sure if the current way abort_page works can help. from http://tcl.apache.org/rivet/manual2.1/abort_page.html 

Synopsis: ::rivet::abort_page (abort code | -aborting)

....

The argument -aborting causes abort_page to return 1 when the current execution is the outcome of an abort condition. In other words this query is meaningful in code specified as AfterEveryScript to understand if an abort condition took place beforehand.
Comment 3 Massimo Manghi 2013-05-22 20:58:52 UTC
For sake of completeness also ErrorScript is uneffective to catch an error that might occur in AfterEveryScript (with either ABORTPAGE or any other error code). AfterEveryScript is actually the very last script that runs in a request processing
Comment 4 Massimo Manghi 2013-05-24 17:45:11 UTC
continuing on the ranting about abort_page, AfterEveryScript and ErrorScript. In case AfterEveryScript fails because of a Tcl error there is no way to catch this error which probably results in the usual backtrace being printed on the browser window and no chance to prevent it. Therefore execution of AfterEveryScript should likewise trigger ErrorScript in case of uncaught errors, opening the question of having abort_page disabled during AfterEveryScript execution or not. If abort_page is not disabled also AbortScript should be in case executed, making the whole request processing slightly more expensive. Karl, what do you think?
Comment 5 Harald Oehlmann 2013-07-02 08:40:06 UTC
Created attachment 30521 [details]
sorry, wrong bug
Comment 6 Massimo Manghi 2015-12-15 14:48:45 UTC
fixed in 2.2.3