{"thread":{"id":"26032","subject":"[RFC PATCH 0/2] gitweb: die_error (error handling) improvements","startedAt":"2010-12-13T00:46:20Z","lastAt":"2010-12-13T09:46:38Z","messageCount":6,"participants":["Jakub Narebski","J.H."],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"157906","messageId":"20101213004259.9475.87376.stgit@localhost.localdomain","threadId":"26032","inReplyTo":null,"subject":"[RFC PATCH 0/2] gitweb: die_error (error handling) improvements","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-13T00:46:20Z","receivedAt":"2010-12-13T00:46:20Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"The following two patch series changes improve error / exception\nhandling in gitweb, preparing the way for gitweb output caching, but\nuseful even without it.\n\nI'm sending this patch series early to gather feedback on possible\nways of improving error / exception handling in gitweb.\n\n\nShortlog:\n~~~~~~~~~\nJakub Narebski (2):\n      gitweb: use eval + die for error (exception) handling\n      gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in die_error\n\nDiffstat:\n~~~~~~~~~\n gitweb/gitweb.perl |   27 +++++++++------------------\n 1 files changed, 9 insertions(+), 18 deletions(-)\n\n-- \nJakub Narebski\nShadeHawk on #git\nPoland\n"},{"id":"157907","messageId":"20101213004644.9475.10102.stgit@localhost.localdomain","threadId":"26032","inReplyTo":"20101213004259.9475.87376.stgit@localhost.localdomain","subject":"[RFC PATCH 1/2] gitweb: Go to DONE_REQUEST rather than DONE_GITWEB in die_error","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-13T00:48:55Z","receivedAt":"2010-12-13T00:48:55Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"End the request after die_error finishes, rather than exiting gitweb\ninstance (perhaps wrapped like in ModPerl::Registry or gitweb.psgi\ncase).\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis patch was sent to git mailing list as a standalone RFC patch some\ntime ago.  This version doesn't change anything from previous version.\n\nI am keeping this patch (even though it is not strictly necessary), to\nhave DONE_REQUEST label, which I think can be quite useful, even if\ndie_error wouldn't be using it starting from the following commit.\n\n gitweb/gitweb.perl |    3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex cfa511c..af45daa 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -1147,6 +1147,7 @@ sub run {\n \n \t\trun_request();\n \n+\tDONE_REQUEST:\n \t\t$post_dispatch_hook->()\n \t\t\tif $post_dispatch_hook;\n \n@@ -3669,7 +3670,7 @@ EOF\n \tprint \"</div>\\n\";\n \n \tgit_footer_html();\n-\tgoto DONE_GITWEB\n+\tgoto DONE_REQUEST\n \t\tunless ($opts{'-error_handler'});\n }\n \n"},{"id":"157908","messageId":"20101213004916.9475.77561.stgit@localhost.localdomain","threadId":"26032","inReplyTo":"20101213004259.9475.87376.stgit@localhost.localdomain","subject":"[RFC PATCH 2/2] gitweb: use eval + die for error (exception) handling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-13T00:49:57Z","receivedAt":"2010-12-13T00:49:57Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"In gitweb code it is assumed that calling die_error() subroutine would\nend request, just like running \"die\" would.  Up till now it was done by\nhaving die_error() jump to DONE_REQUEST (earlier DONE_GITWEB), or in\nearlier version just 'exit' (for mod_perl via ModPerl::Registry it ends\nrequest instead of exiting worker).\n\nInstead of using 'goto DONE_REQUEST' for longjmp-like nonlocal jump, or\nusing 'exit', gitweb uses now native for Perl exception handlingin the\nform of eval / die pair (\"eval BLOCK\" to trap exceptions, \"die LIST\" to\nraise/throw them).\n\nUp till now the \"goto DONE_REQUEST\" solution was enough, but with the\ncoming output caching support and it adding modular structure to gitweb,\nit would be difficult to continue to keep using this solution\n(e.g. interaction with capturing output).\n\n\nBecause gitweb now traps all exceptions occuring run_request(), the\nhandle_errors_html handler (set via set_message from CGI::Carp) is not\nneeded; gitweb can call die_error in -error_handler mode itself.  This\nhas the advantage that we can now set correct HTTP header (handler from\nCGI::Carp::set_message was run after HTTP headers were already sent).\n\nGitweb assumes here that exceptions thrown by Perl would be simple\nstrings; die_error() throws hash reference (if not for minimal\nextrenal dependencies, it would be probable object of Class::Exception\nor Throwable class thrown).\n\nNote: in newer versions of CGI::Carp there is set_die_handler(), where\nhandler have to set HTTP headers to the browser itself, but we cannot\nrely on new enough CGI::Carp version to have been installed.  Also\nset_die_handler interferes with fatalsToBrowser.\n\nSigned-off-by: Jakub Narebski <jnareb@gmail.com>\n---\nThis is main part of this series.\n\nComments?\n\n gitweb/gitweb.perl |   26 ++++++++------------------\n 1 files changed, 8 insertions(+), 18 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex af45daa..ab85c53 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -20,7 +20,7 @@ use lib __DIR__ . '/lib';\n \n use CGI qw(:standard :escapeHTML -nosticky);\n use CGI::Util qw(unescape);\n-use CGI::Carp qw(fatalsToBrowser set_message);\n+use CGI::Carp qw(fatalsToBrowser);\n use Encode;\n use Fcntl ':mode';\n use File::Find qw();\n@@ -1034,21 +1034,6 @@ sub configure_gitweb_features {\n \t}\n }\n \n-# custom error handler: 'die <message>' is Internal Server Error\n-sub handle_errors_html {\n-\tmy $msg = shift; # it is already HTML escaped\n-\n-\t# to avoid infinite loop where error occurs in die_error,\n-\t# change handler to default handler, disabling handle_errors_html\n-\tset_message(\"Error occured when inside die_error:\\n$msg\");\n-\n-\t# you cannot jump out of die_error when called as error handler;\n-\t# the subroutine set via CGI::Carp::set_message is called _after_\n-\t# HTTP headers are already written, so it cannot write them itself\n-\tdie_error(undef, undef, $msg, -error_handler => 1, -no_http_header => 1);\n-}\n-set_message(\\&handle_errors_html);\n-\n # dispatch\n sub dispatch {\n \tif (!defined $action) {\n@@ -1145,7 +1130,11 @@ sub run {\n \t\t$pre_dispatch_hook->()\n \t\t\tif $pre_dispatch_hook;\n \n-\t\trun_request();\n+\t\teval { run_request() };\n+\t\tif (defined $@ && !ref($@)) {\n+\t\t\t# some Perl error, but not one thrown by die_error\n+\t\t\tdie_error(undef, undef, $@, -error_handler => 1);\n+\t\t}\n \n \tDONE_REQUEST:\n \t\t$post_dispatch_hook->()\n@@ -3670,7 +3659,8 @@ EOF\n \tprint \"</div>\\n\";\n \n \tgit_footer_html();\n-\tgoto DONE_REQUEST\n+\n+\tdie {'status' => $status, 'error' => $error}\n \t\tunless ($opts{'-error_handler'});\n }\n \n"},{"id":"157910","messageId":"4D058228.7040905@eaglescrag.net","threadId":"26032","inReplyTo":"20101213004259.9475.87376.stgit@localhost.localdomain","subject":"Re: [RFC PATCH 0/2] gitweb: die_error (error handling) improvements","fromName":"J.H.","fromEmail":"warthog9@eaglescrag.net","sentAt":"2010-12-13T02:17:12Z","receivedAt":"2010-12-13T02:17:12Z","isPatch":true,"sender":{"key":"warthog9@kernel.org","avatar":"https://avatars.githubusercontent.com/u/2334704?v=4"},"body":"On 12/12/2010 04:46 PM, Jakub Narebski wrote:\n> The following two patch series changes improve error / exception\n> handling in gitweb, preparing the way for gitweb output caching, but\n> useful even without it.\n> \n> I'm sending this patch series early to gather feedback on possible\n> ways of improving error / exception handling in gitweb.\n\nPersonally, instead of another band-aid over this problem, and adding\n(or further legitimizing) goto statements inside gitweb I'd much *MUCH*\nrather we actually put in the work to actually clean this up.\n\nThis is the direction I'm heading in, which I mentioned in an earlier\ne-mail.\n\nThere are a *LOT* of disadvantages to the eval mechanism in perl.  It's\nthe standard but gitweb is getting more and more complex, and eval is\nsimplistic.  Couple that with the complexity and uncertainty that things\nlike goto add to the code, I would *MUCH* rather not see this series go\nin, as I think it is the wrong approach to fixing this.\n\n- John 'Warthog9' Hawley\n"},{"id":"157928","messageId":"201012130855.23013.jnareb@gmail.com","threadId":"26032","inReplyTo":"4D058228.7040905@eaglescrag.net","subject":"Re: [RFC PATCH 0/2] gitweb: die_error (error handling) improvements","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-13T07:55:22Z","receivedAt":"2010-12-13T07:55:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 13 Dec 2010, J.H. wrote:\n> On 12/12/2010 04:46 PM, Jakub Narebski wrote:\n\n> > The following two patch series changes improve error / exception\n> > handling in gitweb, preparing the way for gitweb output caching, but\n> > useful even without it.\n> > \n> > I'm sending this patch series early to gather feedback on possible\n> > ways of improving error / exception handling in gitweb.\n> \n> Personally, instead of another band-aid over this problem, and adding\n> (or further legitimizing) goto statements inside gitweb I'd much *MUCH*\n> rather we actually put in the work to actually clean this up.\n\nThat's not band-aid, that's using Perl exception mechanism.  Gitweb\nuses die_error() like one would ordinarily use \"die\".\n \n> This is the direction I'm heading in, which I mentioned in an earlier\n> e-mail.\n\nWell, then how do you want to handle errors?  Note that die_error calls\nare sometimes nested quite deep in the call stack, so using return value\nto denote errors and checking it is rather out of question: it would\nsignificantly increase complexity of code for no gain.\n\nNevertheless I'll take a look how it is solved in other web applications\nwritten in Perl, like SVN::Web or CPAN Hubble.\n \n> There are a *LOT* of disadvantages to the eval mechanism in perl.  It's\n> the standard but gitweb is getting more and more complex, and eval is\n> simplistic.  Couple that with the complexity and uncertainty that things\n> like goto add to the code, I would *MUCH* rather not see this series go\n> in, as I think it is the wrong approach to fixing this.\n\neval / die is not like goto, but like exception mechanism in other\nlanguages.  I'd prefer to use Try::Tiny or TryCatch, but we have this\n\"no extra dependencies\" policy for gitweb.\n-- \nJakub Narebski\nPoland\n"},{"id":"157935","messageId":"201012131046.38725.jnareb@gmail.com","threadId":"26032","inReplyTo":"201012130855.23013.jnareb@gmail.com","subject":"Re: [RFC PATCH 0/2] gitweb: die_error (error handling) improvements","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-12-13T09:46:38Z","receivedAt":"2010-12-13T09:46:38Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 13 Dec 2010, Jakub Narebski wrote:\n\n> Nevertheless I'll take a look how it is solved in other web applications\n> written in Perl, like SVN::Web or CPAN Hubble.\n\nSVN::Web uses eval / die... well, it uses SVN::Web::X->throw() instead\nof \"die\" for error handling (SVN::Web::X is based on Exception::Class;\nother class to use for exceprion handling is Throwable but it requires\nMoose).  For errors early in the process it just uses \"die\".\n\n-- \nJakub Narebski\nPoland\n"}]}