Closed Bug 1089475 Opened 6 years ago Closed 6 years ago

Use "ThrowCodeError" when a database error occurs instead of dumping a stack trace

Categories

(Bugzilla :: Database, enhancement)

4.5.6
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 6.0

People

(Reporter: Jeff.Fearn, Assigned: Jeff.Fearn)

References

Details

Attachments

(1 file, 6 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0
Build ID: 20141013035207

Steps to reproduce:

Ran the query on Bug 889650




Actual results:

The search creates invalid SQL, which breaks the user out of the UI and leaves them in an invalid state.

When running under CGI Firefox reports an incorrect encoding error, which it isn't, and leaves the user without a UI.

When running under mod_perl firefox reports a 500 and leaves the user on a page with a permanent please wait message, again without a UI. Since the thread has ended there will be no further data and the user will wait until they are angry before hitting the back button.



Expected results:

The error should have been caught and reported without breaking the user out of the BZ UI.
Assignee: general → Jeff.Fearn
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch 1089475.patch (obsolete) — Splinter Review
Switch from default error handlers to ThrowCodeError. Add new message to global error template.

The result is a nice error page with the correct message in the BZ UI. Tested under CGI and mod_perl.
Attachment #8511807 - Flags: review?(glob)
Summary: SQL errors break the UI → Use "ThrowCodeError" when a database error occurs instead of dumping a stack trace
Comment on attachment 8511807 [details] [diff] [review]
1089475.patch

Review of attachment 8511807 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/DB.pm
@@ +137,5 @@
>  }
>  
>  sub _handle_error {
> +    ThrowCodeError("database_call_failed", {err_message => $_[0]});
> +    return 1; # Tell DBI error has been handled

the return is not required; ThrowCodeError doesn't return.

::: template/en/default/global/code-error.html.tmpl
@@ +388,5 @@
>      "[% operator FILTER html %]".
>  
> +  [% ELSIF error == "database_call_failed" %]
> +    Database call failed.
> +    <pre>[% err_message FILTER none %]</pre>

this error should be called db_error to match the existing database error code, and should be inserted before db_rename_conflict.

"database call failed" isn't a very useful error message to most users.
how about "An error occurred while performing a database operation:"
Attachment #8511807 - Flags: review?(glob) → review-
Attached patch 1089475-2.patch (obsolete) — Splinter Review
Fixes issues raised in original patch.
Attachment #8511807 - Attachment is obsolete: true
Attachment #8511817 - Flags: review?(glob)
Attached patch 1089475-3.patch (obsolete) — Splinter Review
D'oh, missed filter :/
Attachment #8511817 - Attachment is obsolete: true
Attachment #8511817 - Flags: review?(glob)
Attachment #8511818 - Flags: review?(glob)
Comment on attachment 8511818 [details] [diff] [review]
1089475-3.patch

>+++ b/Bugzilla/DB.pm

> sub _handle_error {

>-    # Cut down the error string to a reasonable size
>-    $_[0] = substr($_[0], 0, 2000) . ' ... ' . substr($_[0], -2000)
>-        if length($_[0]) > 4000;

The length of the error message was limited to 4000 characters on purpose, see bug 256592. No reason to revert this limitation.


>-    $_[0] = Carp::longmess($_[0]);
>-    return 0; # Now let DBI handle raising the error
>+    ThrowCodeError("db_error", {err_message => $_[0]});

Till now, we were simply limiting the size of the error message, and let DBI handle the error itself. Now you short-circuit it and throw the error yourself. Are you sure this doesn't prevent some internal DBI stuff from happening when an error is triggered? The DBI documentation doesn't mention anything about that.

Also, you have no guarantee that Template-Toolkit is ready to display text as checksetup.pl runs DB stuff before compiling templates.
Attachment #8511818 - Flags: review?(glob) → review-
Severity: normal → enhancement
Component: Bugzilla-General → Database
Attached patch 1089475-4.patch (obsolete) — Splinter Review
Re-added 4K length limit. Only use ThrowCodeError in web mode.

HandleError is run late in the DBI process as it's for telling DBI to mask, or not mask, the error. As ThrowCodeError calls rollback it should be safe, but given DBI has already errored it's unlikely any data could be lost at this point.
Attachment #8511818 - Attachment is obsolete: true
Attachment #8512313 - Flags: review?(glob)
Attached patch 1089475-5.patch (obsolete) — Splinter Review
Remove some extraneous test content from the template :/

Re-added 4K length limit. Only use ThrowCodeError in web mode.

HandleError is run late in the DBI process as it's for telling DBI to mask, or not mask, the error. As ThrowCodeError calls rollback it should be safe, but given DBI has already errored it's unlikely any data could be lost at this point.
Attachment #8512313 - Attachment is obsolete: true
Attachment #8512313 - Flags: review?(glob)
Attachment #8512315 - Flags: review?(glob)
Comment on attachment 8512315 [details] [diff] [review]
1089475-5.patch

Review of attachment 8512315 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob
Attachment #8512315 - Flags: review?(glob) → review+
thanks jeff!

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f1d4771..bd4dcb0  master -> master
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: approval+
Resolution: --- → FIXED
Target Milestone: --- → Bugzilla 6.0
This is live now.
Blocks: 1128856
Reopening! This totally breaks Bugzilla when it cannot reach the DB server. CPU is at 100% forever, and the error log shows:

Deep recursion on subroutine "Bugzilla::dbh" at Bugzilla/Error.pm line 39.
[...]
Deep recursion on subroutine "DBI::connect" at Bugzilla/DB.pm line 1275.

The reason is that Bugzilla::DB::_handle_error() now calls ThrowCodeError() which in turn calls Bugzilla->dbh which cannot access the DB and so triggers _handle_error() again, etc... in an endless loop. I had to manually kill the process. Not good!

Instead of ThrowCodeError(), calling die would be better and would avoid deep recursions.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I backed out the patch:

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   721ce16..e6d2fb7  master -> master
Flags: approval+
Yeah, it needs to die if it gets run more than once.
(In reply to Jeff Fearn from comment #13)
> Yeah, it needs to die if it gets run more than once.

Jeff: We had this problem with Red Hat Bugzilla too. IIRC, either Matt or myself wrote a patch to get around it. As I don't have access to the code, I'm not sure what it is was.
Hi, is it a reasonable solution to this to set "generate_api_token = 0" in code-error.html.tmpl when loading header.html.tmpl?

I can't think of a reason you'd want to enable token creation on code error pages.
(In reply to Jeff Fearn from comment #15)
> Hi, is it a reasonable solution to this to set "generate_api_token = 0" in
> code-error.html.tmpl when loading header.html.tmpl?
> 
> I can't think of a reason you'd want to enable token creation on code error
> pages.

sounds reasonable to me.
Attached patch avoid recursion (obsolete) — Splinter Review
OK so the way I was going to do it won't work because a lot of templates expect the DB to be there and won't work without it. i.e. I was getting errors about invalid headers and such as the templates failed to generate a valid response.

Tested with no DB and a read only DB, no DB displays the original error screen, RO DB displays a shiny SQL error about not being allowed to update the login cookie table.

nasty global var but I couldn't think of another way around it, suggestions welcome :}
Attachment #8512315 - Attachment is obsolete: true
Attachment #8610295 - Flags: review?(glob)
Comment on attachment 8610295 [details] [diff] [review]
avoid recursion

Review of attachment 8610295 [details] [diff] [review]:
-----------------------------------------------------------------

global variables are not mod_perl friendly.
(ab)use the request_cache instead.
Attachment #8610295 - Flags: review?(glob) → review-
Attachment #8610295 - Attachment is obsolete: true
Attachment #8610335 - Flags: review?(glob)
Comment on attachment 8610335 [details] [diff] [review]
mod_perl friendly avoid recursion

Review of attachment 8610335 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob
Attachment #8610335 - Flags: review?(glob) → review+
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   4ee100d..db6b722  master -> master
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: approval+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.