Skip to content
This repository has been archived by the owner on Jan 25, 2019. It is now read-only.

Unhandled exception handler is missing, causing application to crash when happened #6

Closed
leledumbo opened this issue Dec 30, 2012 · 33 comments
Assignees

Comments

@leledumbo
Copy link
Collaborator

In case of unhandled exception occurs, a brook app will crash and causes HTTP 502, while in fpweb, it will generate a backtrace similar as in desktop application, only converted as html and sent as response. The application is still alive after that. Please add this functionality. You can take a look at custweb unit as an implementation reference. I tried making it as pluggable module but apparently I can't do any output from the exception handler and I think the better way is to make it integrated in the framework.

@silvioprog
Copy link
Owner

Fixed (c8f0ceb).

To test it, implement:

unit Brokers;

{$mode objfpc}{$H+}

interface

uses
  BrookFCLCGIBroker, BrookUtils;

implementation

initialization
  BrookSettings.Page404 :=
    '<html><head><title>Page not found</title></head><body>' +
    '<h1>404 - Page not found</h1></body></html>';

end.

And, to causing the error:

type
  TMyAction = class(TBrookAction)
  public
    procedure Get; override;
  end;

implementation

procedure TMyAction.Get;
begin
  Error('Fail');
end;

You will got a default FCL-Web content, sush as:

: ERROR

The application encountered the following error:
Error: TMyAction: Fail
Stack trace:
$00436821 TBROOKACTION__ERROR, line 500 of C:/repository/git/brookframework-silvioprog/core/brookaction.pas
$00411EF6 TMYACTION__GET, line 20 of unit1.pas
$00436478 TBROOKACTION__REQUEST, line 401 of C:/repository/git/brookframework-silvioprog/core/brookaction.pas
$00436298 TBROOKACTION__DOREQUEST, line 338 of C:/repository/git/brookframework-silvioprog/core/brookaction.pas
$0042D985 TBROOKROUTER__DOEXECUTEACTION, line 326 of C:/repository/git/brookframework-silvioprog/core/brookrouter.pas
$0042E3CE TBROOKROUTER__ROUTE, line 606 of C:/repository/git/brookframework-silvioprog/core/brookrouter.pas
$0042CA77 TBROOKCGIHANDLER__HANDLEREQUEST, line 258 of C:/repository/git/brookframework-silvioprog/brokers/brookfclcgibroker.pas
$004323E1
$0042C1E4 TBROOKAPPLICATION__RUN, line 108 of C:/repository/git/brookframework-silvioprog/brokers/brookfclcgibroker.pas
$00401727 main, line 9 of cgi1.lpr
Notify: webmaster: webmaster@localhost

@leledumbo
Copy link
Collaborator Author

I don't get the backtrace, the application is still terminated on error. Note that I'm using FastCGI.

@silvioprog
Copy link
Owner

I'll test it. Please send me a small demo using fpWeb with FastCGI.

@leledumbo
Copy link
Collaborator Author

silvioprog added a commit that referenced this issue Jan 1, 2013
@silvioprog
Copy link
Owner

Fixed (15f0141). Please test if it's OK.

@leledumbo
Copy link
Collaborator Author

I can't merge this change automatically to my fork. Maybe you could?

@silvioprog
Copy link
Owner

Hm... I have not found an option for that. :/ You can delete the project and fork again.

@leledumbo
Copy link
Collaborator Author

Ah..I see that you've given write access for me, that's why. I'll do direct push from now as the way to manually merge involve some git commands I never use and must be done from terminal.

@leledumbo
Copy link
Collaborator Author

OK, everything works now. I guess you need to document that in order the stack trace to appear, BrookSettings.PageXXX must be empty string.

@silvioprog
Copy link
Owner

I fully agree. So, my friend who translates from Portuguese to English is on vacation and returns just in February, could you help me on this?

@leledumbo
Copy link
Collaborator Author

Which source holds the documentation? Or where should I add this documentation?

@silvioprog
Copy link
Owner

You can edit it directly in the .pas:

1 - https://github.com/silvioprog/brookframework/blob/master/core/brookutils.pas#L49

2 - https://github.com/silvioprog/brookframework/blob/master/core/brookutils.pas#L52

And after, you or I re-generate the HTML via PasDoc to upload it to the site. :)

@silvioprog
Copy link
Owner

Nice?:

{ Set the 500 HTML page. It can be a HTML string, a file pack,
  a JSON string, etc. If this property is empty, the Brook use
  the default error page of the FCL-Web. }

@leledumbo
Copy link
Collaborator Author

I'd add:

the (or then?) Brook use the default error page of the FCL-Web, which may contain stack trace.

But reading about it... I'm thinking about a setting or property to allow stack trace to appear regardless the Page404 or Page500 is filled or not. Guess it would be easy to implement, I'll try something tonight (I have no git at office).

@silvioprog
Copy link
Owner

This would be an interesting feature.
I use my own 4xx/5xx pages, but would like to enable or disable the stack trace (to discover bugs without run the CGIs in the terminal).

@mdbs99
Copy link
Contributor

mdbs99 commented Jan 4, 2013

This new feature is very important. I think a property is sufficient as leledumbo said.

@leledumbo
Copy link
Collaborator Author

Implemented: 81492b2

@mdbs99
Copy link
Contributor

mdbs99 commented Jan 5, 2013

So, closed.

@mdbs99 mdbs99 closed this as completed Jan 5, 2013
@silvioprog
Copy link
Owner

I think I'll have to change this commit, because the user (programmer) may be using purely JSON, and it would show an error message in HTML into JSON content, and this would be a problem. :/
I have not had time to test this change because I'm on my girlfriend's PC, but I'll test as soon as possible.
I think that just adding the stack trace into the error message would meet everyone (JSON users, HTML users or both).

@silvioprog silvioprog reopened this Jan 5, 2013
@silvioprog
Copy link
Owner

Another interesting option would be to add an additional input format, an example of use would be:

  BrookSettings.Page500 :=
    '<html><head><title>Internal server error</title></head><body>' +
    '<h1>500 - Internal server error</h1>' +
    '<p style="color: red;" >Error: %s</p>' +
    '<p style="color: red;" >Stack trace: %s</p></body></html>';

I.e.: The first %s is the error message, and the last %s is the stack trace, so it would be used in JSON error too, e.g.:

BrookSettings.Page500 := '{ "error": "%s", "trace": "%s" }';

Nice?

@mdbs99
Copy link
Contributor

mdbs99 commented Jan 5, 2013

Maybe:

  BrookSettings.Page500 :=
    '<html><head><title>Internal server error</title></head><body>' +
    '<h1>500 - Internal server error</h1>' +
    '<p style="color: red;" >Error: #ERROR#</p>' +
    '<p style="color: red;" >Stack trace: #STACKTRACE#</p></body></html>';

and

BrookSettings.Page500 := '{ "error": "#ERROR#", "trace": "#STACKTRACE#" }';

So, using "macros". The user can use whatever information he want.

@silvioprog
Copy link
Owner

Or, using JTemplate's syntax::

BrookSettings.Page500 :=
    '<html><head><title>Internal server error</title></head><body>' +
    '<h1>500 - Internal server error</h1>' +
    '<p style="color: red;" >Error: @error</p>' +
    '<p style="color: red;" >Stack trace: @trace</p></body></html>';
BrookSettings.Page500 := '{ "error": "@error", "trace": "@trace" }';

Good?

@mdbs99
Copy link
Contributor

mdbs99 commented Jan 6, 2013

For me is the same thing. The most import is the idea, not the implementation... in this case, of course.

@leledumbo
Copy link
Collaborator Author

If you replace with a SysUtils.Format() style function, it might inadequate (because the search is done by index), but with JTemplate style it's better (search by name).
A bit OOT: Why JTemplate instead of FPTemplate / TTemplateParser?

@mdbs99
Copy link
Contributor

mdbs99 commented Jan 6, 2013

If you replace with a SysUtils.Format() style function, it might inadequate (because the search is done by index),
but with JTemplate style it's better (search by name).

Because this I suggested to use 'macros' (but doesn't matter which engine will be used).

A bit OOT: Why JTemplate instead of FPTemplate / TTemplateParser?

I do not know.

@leledumbo
Copy link
Collaborator Author

A bit OOT: Why JTemplate instead of FPTemplate / TTemplateParser?

The question is for Silvio actually :3

@silvioprog
Copy link
Owner

About the 'macros', there are great chance to use the JTemplate sintax, because it is very simple and straightforward.

About JTpl X FPTpl X TplParser: IMHO, I think the JTemplate more fast, sleek and intuitive, and it offers a native support for JSON and HTML.

silvioprog added a commit that referenced this issue Jan 7, 2013
@silvioprog
Copy link
Owner

Implemented in "master" branch (the new "tmp" changes will be implemented in "working" branch): 57921e5.

It is implemented using SysUtils.Format(), but in the future it can be implemented using macros.

So, for this configuration:

(...)
  BrookSettings.Page500 :=
    '<html><head><title>Internal server error</title></head><body>' +
    '<h1>500 - Internal server error</h1>' +
    '<p style="color: red;" >Error: %s</p>' +
    '<p style="color: red;" >Trace: %s</p></body></html>';
(...)

The result is:

<html><head><title>Internal server error</title></head><body><h1>500 - Internal server error</h1><p style="color: red;" >Error: TMyAction: Fail</p><p style="color: red;" >Trace:   $004369F1  TBROOKACTION__ERROR,  line 500 of C:/repository/git/brookframework-silvioprog/core/brookaction.pas<br />  $00411F06  TMYACTION__GET,  line 20 of unit1.pas<br />  $00436648  TBROOKACTION__REQUEST,  line 401 of C:/repository/git/brookframework-silvioprog/core/brookaction.pas<br />  $00436468  TBROOKACTION__DOREQUEST,  line 338 of C:/repository/git/brookframework-silvioprog/core/brookaction.pas<br />  $0042DB55  TBROOKROUTER__DOEXECUTEACTION,  line 326 of C:/repository/git/brookframework-silvioprog/core/brookrouter.pas<br />  $0042E59E  TBROOKROUTER__ROUTE,  line 606 of C:/repository/git/brookframework-silvioprog/core/brookrouter.pas<br />  $0042CA87  TBROOKCGIHANDLER__HANDLEREQUEST,  line 258 of C:/repository/git/brookframework-silvioprog/brokers/brookfclcgibroker.pas<br />  $004325B1<br />  $0042C1F4  TBROOKAPPLICATION__RUN,  line 108 of C:/repository/git/brookframework-silvioprog/brokers/brookfclcgibroker.pas<br />  $00401727  main,  line 9 of project1.lpr<br /></p></body></html>

And for this:

(...)
  BrookSettings.ContentType := BROOK_HTTP_CONTENT_TYPE_APP_JSON;
(...)
  BrookSettings.Page500 := '{ "error": "%s", "trace": "%s" }';  
(...)

The result is:

{ "error": "TMyAction: Fail", "trace": "  $00436A01  TBROOKACTION__ERROR,  line 500 of C:\/repository\/git\/brookframework-silvioprog\/core\/brookaction.pas\r\n  $00411F16  TMYACTION__GET,  line 20 of unit1.pas\r\n  $00436658  TBROOKACTION__REQUEST,  line 401 of C:\/repository\/git\/brookframework-silvioprog\/core\/brookaction.pas\r\n  $00436478  TBROOKACTION__DOREQUEST,  line 338 of C:\/repository\/git\/brookframework-silvioprog\/core\/brookaction.pas\r\n  $0042DB65  TBROOKROUTER__DOEXECUTEACTION,  line 326 of C:\/repository\/git\/brookframework-silvioprog\/core\/brookrouter.pas\r\n  $0042E5AE  TBROOKROUTER__ROUTE,  line 606 of C:\/repository\/git\/brookframework-silvioprog\/core\/brookrouter.pas\r\n  $0042CA97  TBROOKCGIHANDLER__HANDLEREQUEST,  line 258 of C:\/repository\/git\/brookframework-silvioprog\/brokers\/brookfclcgibroker.pas\r\n  $004325C1\r\n  $0042C204  TBROOKAPPLICATION__RUN,  line 108 of C:\/repository\/git\/brookframework-silvioprog\/brokers\/brookfclcgibroker.pas\r\n  $00401727  main,  line 9 of project1.lpr\r\n" }

@leledumbo
Copy link
Collaborator Author

Hmm.. since SysUtils.Format is used, if the user doesn't supply the required %s (or if one supplies another %), it will cause a runtime error. Then how NOT to get the stack trace?

@silvioprog
Copy link
Owner

You can omit the %s, e.g.:

initialization
  BrookSettings.Page404 :=
    '<html><head><title>Page not found</title></head><body>' +
    '<h1>404 - Page not found</h1></body></html>';
  BrookSettings.Page500 :=
    '<html><head><title>Internal server error</title></head><body>' +
    '<h1>500 - Internal server error</h1>' +
    '<p style="color: red;" >Error: %s</p></body></html>'; 

The result:

<html><head><title>Internal server error</title></head><body><h1>500 - Internal server error</h1><p style="color: red;" >Error: TMyAction: Fail</p></body></html>

Omitting both %s:

  BrookSettings.Page500 :=
    '<html><head><title>Internal server error</title></head><body>' +
    '<h1>500 - Internal server error</h1>' +
    '<p style="color: red;" >Oh God, why? :(</p></body></html>'; 

Result:

<html><head><title>Internal server error</title></head><body><h1>500 - Internal server error</h1><p style="color: red;" >Oh God, why? :(</p></body></html>

But it will be implemented with macros soon. :)

@silvioprog
Copy link
Owner

So, an implementation for macro support:

procedure TBrookCGIHandler.ShowRequestException(R: TResponse; E: Exception);
var
  VStr: TStrings;
  VHandled: Boolean = False;
begin
  if R.ContentSent then
    Exit;
  if Assigned(BrookSettings.OnError) then
  begin
    BrookSettings.OnError(R, E, VHandled);
    if VHandled then
      Exit;
  end;
  if Assigned(OnShowRequestException) then
  begin
    OnShowRequestException(R, E, VHandled);
    if VHandled then
      Exit;
  end;
  if RedirectOnError and not R.HeadersSent then
  begin
    R.SendRedirect(Format(RedirectOnErrorURL, [HTTPEncode(E.Message)]));
    R.SendContent;
    Exit;
  end;
  if (BrookSettings.Page404 <> ES) and (E is EBrookHTTP404) and
    (not R.HeadersSent) then
  begin
    R.Code := BROOK_HTTP_STATUS_CODE_NOT_FOUND;
    R.CodeText := BROOK_HTTP_REASON_PHRASE_NOT_FOUND;
    R.ContentType := FormatContentType;
    if FileExists(BrookSettings.Page404) then
      R.Contents.LoadFromFile(BrookSettings.Page404)
    else
      R.Content := BrookSettings.Page404;
    R.Content := Format(R.Content, [ApplicationURL]);
    R.SendContent;
    Exit;
  end;
  if (BrookSettings.Page500 <> ES) and (not R.HeadersSent) then
  begin
    R.Code := BROOK_HTTP_STATUS_CODE_INTERNAL_SERVER_ERROR;
    R.CodeText := 'Application error ' + E.ClassName;
    R.ContentType := FormatContentType;
    if FileExists(BrookSettings.Page500) then
    begin
      R.Contents.LoadFromFile(BrookSettings.Page500);
      R.Content := StringsReplace(R.Content, ['@error', '@trace'],
        [E.Message, BrookDumpStack], [rfIgnoreCase, rfReplaceAll]);
    end
    else
      if BrookSettings.ContentType = BROOK_HTTP_CONTENT_TYPE_APP_JSON then
        R.Content := StringsReplace(BrookSettings.Page500, ['@error', '@trace'],
          [StringToJSONString(E.Message), StringToJSONString(BrookDumpStack(LE))],
          [rfIgnoreCase, rfReplaceAll])
      else
        R.Content := StringsReplace(BrookSettings.Page500, ['@error', '@trace'],
          [E.Message, BrookDumpStack], [rfIgnoreCase, rfReplaceAll]);
    R.SendContent;
    Exit;
  end;
  if (R.ContentType = BROOK_HTTP_CONTENT_TYPE_TEXT_HTML) or
    (BrookSettings.ContentType = BROOK_HTTP_CONTENT_TYPE_TEXT_HTML) then
  begin
    VStr := TStringList.Create;
    try
      ExceptionToHTML(VStr, E, Title, Email, Administrator);
      R.Content := VStr.Text;
      R.SendContent;
    finally
      VStr.Free;
    end;
  end;
end;

Using StrUtils.StringsReplace() sounds interesting?

@leledumbo
Copy link
Collaborator Author

That's better, since there will be only 2 placeholders, I think StrUtils.StringsReplace is fine.

silvioprog added a commit that referenced this issue Jan 7, 2013
silvioprog added a commit that referenced this issue Jan 7, 2013
silvioprog added a commit that referenced this issue Jan 7, 2013
@silvioprog
Copy link
Owner

Done in branch "working".
1 - 379f7db
2 - a6659af
3 - 6138c02

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants