When an exceptions tries to bubble out of a shutdown function or the errorhandler (or autoload functions, but they are not handled yet), the following happens:

Fatal error: Exception thrown without a stack frame in Unknown on line 0

This is hell to debug, because you don't have a clue where the exception happened. It could be a hook_cron(), something with the session, a borked database table or something else.

This patch catches these and prints the actual exception message. Not that the message is currently simply printed, it's pretty much only a proof of concept and I'm looking for better ways to do it.

To do this for shutdown functions, the patch adds a new function called drupal_register_shutdown_function(), which registers a single function called _drupal_shutdown_function() if required so. All registered functions are then executed in that function inside a try/catch block.

For more information, see also #566494-167: Running cron in a shutdown function breaks the site

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Title: Handle exceptions in shutdown functions and exception handler » Handle exceptions in shutdown functions
FileSize
7.13 KB

Ok, let's only handle shutdown functions here, re-rolled the patch.

Status: Needs review » Needs work

The last submitted patch, catch_exceptions2.patch, failed testing.

moshe weitzman’s picture

+++ includes/bootstrap.inc
@@ -2356,3 +2356,56 @@ function drupal_is_cli() {
+ * @param $parameter
+ *   It is possible to pass parameters to the shutdown function by passing
+ *   additional parameters.
+ *

parameters should be plural

+++ includes/bootstrap.inc
@@ -2356,3 +2356,56 @@ function drupal_is_cli() {
+  $callbacks = &drupal_static('drupal_shutdown_functions', array());
+  $callbacks_arguments = &drupal_static('drupal_shutdown_functions:arguments', array());

I would personally store these in one static but I'm not too fussed about that.

+++ includes/bootstrap.inc
@@ -2356,3 +2356,56 @@ function drupal_is_cli() {
+    print t('%type: %message in %function (line %line of %file).', _drupal_decode_exception($exception));

t() may not be available. consider cached pages. try something like:

<?php
$t = function_exists('t') ? 't' : 'strtr'

$t(some string)

Powered by Dreditor.

moshe weitzman’s picture

Priority: Normal » Critical

This is a major DX regression.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
7.15 KB

This patch helped me find an mysterious exception in devel's shutdown. This feature is mandatory now that we have exceptions. It should go in no matter what happens with poormanscron.

Fixed those issues and posted new patch.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Actually, I made minor plural change and minor t() change. Berdir's patch is RTBC. Credit him.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, except.diff, failed testing.

moshe weitzman’s picture

test failure in simpletest module. i have no idea what it means tho. help.

Berdir’s picture

I'm working on that. Can someone also point boombatower to this issue as he knows that simpletest/batch api stuff hopefully well? :)

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.36 KB

Ok, this was ugly to find but pretty much logical.

We can't rely on drupal_static() in these functions because that is deleted during batch process somewhere.

The new patch uses a helper function which stores the callbacks in a normal static array.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Nice detective work ... Please wait for green before commit (we are not reporting status back to project-issue module these days?).

chx’s picture

Status: Reviewed & tested by the community » Needs work

It truly is awesome but drupal_register_shutdown_function could keep the static instead of the shared getter as its return value is not used now. Make it so that if you dont pass in anything it does nothing but always returns its static. You don't even need references, woohoo. Nice job, dude.

Berdir’s picture

Status: Needs work » Needs review
FileSize
7.19 KB

Ok, let's try this.

Changed as suggested above by chx. It still needs references though, for two reasons.

- This basically allows contrib to replace core shutdown functions with their own, by getting them and replacing bya specific function ;)
- Some modules (even core, now that cron is called in a shutdown function) like devel.module register new shutdown functions in a shutdown function.

chx’s picture

Status: Needs review » Needs work

If that's the case then foreach wont work, you need a while(list=each) loop because foreach copies.

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.04 KB

Yes, of course :)

I thought the same but wasn't sure as I've seen one case where clon_cleanup has been called. but that was probably because simpletest calls drupal_run_cron() directly when installing a test env.

I also added a simple test case to confirm this and it passes with the while while it failed with a foreach. Not sure if the test can be improved, though... I could add a drupal_static_reset() call but I'm not sure how much that actually helps...

Berdir’s picture

FileSize
10.09 KB

Changed to t() instead of check_plain() in the dummy shutdown functions...

chx’s picture

Status: Needs review » Reviewed & tested by the community

Yay!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, catch_exceptions6.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#16: catch_exceptions6.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

chx and moshe had set this to rtbc, status was reset because of a test failure (which seemed to be a test client hickup as it works now, after a re-test). Who could ask for more ? ;)

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +webchick's D7 alpha hit list

I think now that I committed #325169-94: Move error/exception handler higher up in the bootstrap process, we no longer need that weird t() / strtr check. Could someone double-check?

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
10.02 KB

Removed $t and grabbed this excellent opportunity to change the !is_null to isset.

moshe weitzman’s picture

Status: Reviewed & tested by the community » Needs work

_drupal_decode_exception() may not be available. That only gets included with full bootstrap AFAICT. Perhaps include errors.inc before using _drupal_decode_exception()?

@param $parameter - should be plural (my fault).

Berdir’s picture

Status: Needs work » Needs review
FileSize
10.09 KB

Included errors.inc when necessary and changed to $parameters.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

good for me. WFGBC

webchick’s picture

Status: Reviewed & tested by the community » Fixed

WFGBC. Do I want to know what that stands for? ;)

Committed to HEAD. Thanks a lot, folks!

Berdir’s picture

Wait for green before commit :)

webchick’s picture

Ahhh. :) Thanks!

andypost’s picture

Status: Fixed » Needs work

Introduced drupal_register_shutdown_function() statically caches handlers so it breaks simpletest:

- When running a interactive tests - handlers from testing setup in drupal_register_shutdown_function() statically
- When Simpletests clears data there's no ability to clear handlers

To reproduce simply use minimal install or just uninstall search module and get

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /batch?id=13&op=do StatusText: OK ResponseText: {"status":true,"percentage":"100","message":"Processed test 1 of 1 - \u003cem class=\"placeholder\"\u003eActions configuration\u003c\/em\u003e.\u003cdiv class=\"simpletest-pass\"\u003eOverall results: 38 passes, 0 fails, and 0 exceptions\u003c\/div\u003e\u003cdiv class=\"item-list\"\u003e\u003cul\u003e\u003cli class=\"first last\"\u003e\u003cdiv class=\"simpletest-pass\"\u003eActions configuration: 38 passes, 0 fails, and 0 exceptions\u003c\/div\u003e\u003c\/li\u003e\n\u003c\/ul\u003e\u003c\/div\u003e"}PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd72.search_total' doesn't exist: SELECT t.word AS realword, i.word FROM {search_total} t LEFT JOIN {search_index} i ON t.word = i.word WHERE i.word IS NULL; Array ( ) in search_update_totals() (line 371 of /var/www/d7/modules/search/search.module).
andypost’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)
Issue tags: -webchick's D7 alpha hit list

Automatically closed -- issue fixed for 2 weeks with no activity.