drush_shutdown calls exit and preempts all other shutdown functions, breaking search cron and similar [#503328]

Drush register_shutdown_function()'s drush_shutdown at the very beginning of the script, meaning it is going to be the first shutdown function to run. It calls exit(status), however, meaning no other shutdown functions are called. This is a big issue because (for instance) when running drush cron, regular cron jobs use register_shutdown_function() extensively, case in point being the search module which exclusively uses a shutdown function to fill the required search_total table.

This patch makes drush_shutdown() register a little minion shutdown function that returns the status to the command line or wherever with exit(status);. This way, that one will be the last shutdown function called PROVIDED we assume no other shutdown functions register new shutdown functions...I know of no cases of this.

If we wanted to be more paranoid we could just not call exit, but I think that might break some peoples' use of drush's return value.

Thanks,
David

Comment	File	Size	Author
	drush_exit.patch	818 bytes	David Goode

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

moshe weitzman CreditAttribution: moshe weitzman commented 27 June 2009 at 02:23

devel module uses this same technique and it works well. this should still get some review. from the diff i am surprised to only see an add and not a delete for exit((drush_get_error()) ? DRUSH_FRAMEWORK_ERROR : DRUSH_SUCCESS);

Comment #2

adrian CreditAttribution: adrian commented 30 June 2009 at 01:02

Status:

Needs review

» Fixed

committed

Comment #3

David Goode CreditAttribution: David Goode commented 1 July 2009 at 12:57

Hey Moshe,

The reason I didn't get rid of the exit() command was to preserve drush's functionality of returning an error code if applicable, which I later learned was very important for many scripted applications of the module. The only way I know of doing this is by passing an argument when calling exit() and killing the script. This method should work though because shutdown functions are a first-in-first-out queue type of system. The original shutdown function was registered first at the very beginning of drush's initialization, and by calling exit() it stopped the script before any other shutdown functions could run (as per the documented functionality in PHP whereby exit() within a shutdown functions just stops the script cold, whereas anywhere else an exit() command would allow the shutdown functions to run first). Now though, this first shutdown function adds a new shutdown function which will always be the last one registered assuming no other subsequent shutdown functions also create "minions" :-) This way, the main drush shutdown runs but doesn't exit, all other shutdown functions get a chance to run, and finally the last shutdown function exit()'s and returns the same status it would have before the patch.

Thanks to both of you for your time and to Adrian for committing this.

David