hook_exit() not invoked for some cached requests

The main difference between normal and "aggressive" caching is that hook_boot() and hook_exit() are called in normal caching and not in aggressive caching. Given that, I don't understand the issue you are facing.

Ok, understood. Here is a patch (candidate for the trivial patch of the month).

This needs to be changed in drupal_page_cache_header. The situation being described is hitting the following logic in drupal_page_cache_header:

  if ($if_modified_since && $if_none_match
      && $if_none_match == $etag // etag must match
      && $if_modified_since == $last_modified) {  // if-modified-since must match
    header($_SERVER['SERVER_PROTOCOL'] . ' 304 Not Modified');
    // All 304 responses must send an etag if the 200 response for the same object contained an etag
    header("Etag: $etag");
    exit();
  }

Note that the exit() call will not allow the hook_exit call to be made from _drupal_bootstrap:

    case DRUPAL_BOOTSTRAP_LATE_PAGE_CACHE:
      ...
      // If the skipping of the bootstrap hooks is not enforced, call hook_boot.
      if ($cache_mode != CACHE_AGGRESSIVE) {
        bootstrap_invoke_all('boot');
      }
      // If there is a cached page, display it.
      if ($cache) {
        drupal_page_cache_header($cache);
        // If the skipping of the bootstrap hooks is not enforced, call hook_exit.
        if ($cache_mode != CACHE_AGGRESSIVE) {
          bootstrap_invoke_all('exit');
        }
        // We are done.
        exit;
      }

Attached patch changes the exit in drupal_page_cache_header to return, which will allow processing to return to _drupal_bootstrap and fire the exit hook.

Patch in #5 is right. I changed the wrong exit() (oups) in #4 :)

This bug will need to be backported to D6 and D5 as well, so I've made patches for those versions.

Also I think an exit hook (at least in 7.x+ since this would be an API change) needs to be able to find out if it has been invoked after a cached page or header response, or after full page generation.

@gpk: This has nothing to do with this issue. hook_exit() have to call drupal_bootstrap() themselves if they need a fully bootstrap environment (it's harmless to run it in an already bootstrapped environment, so the fix to #231724 seems trivial.

poormanscron_exit should be able to run this check to see if the page is being served cached:

global $user;
if ($user->uid || variable_get('cache', CACHE_DISABLED) == CACHE_DISABLED || drupal_set_header()) {
  // Run!
}

@gpk: _drupal_full_bootstrap() (or drupal_set_header) should learn to be more clever. Open a new issue so that we can improve them :)

Tiny tiny patch. Not sure if this needs tests, but it's ready. Marking as RTBC.

Shortening title.

A more accurate title.

And the executive summary:

When a request for a cached page can be answered with a 304 Not modified response code, drupal_page_cache_header() incorrectly stops the execution of the script, preventing hook_exit() to be fired.

Should be backported to D5.

I'm going to assume that this can be marked as fixed as I just saw this core commit. You can shoot me if I'm wrong.

Ah, shoot. Yeah, I did commit this. Drupal.org's been a little funky tonight. Thanks. :)

However, this should actually be moved down to 6.x.

This is going to have a massive effect on statistics module. It will start counting cached visits which is good for reporting but not so good for performance. Statistics itself is not a good idea for high traffic sites so i am not too worried about this change.

Agreed with Moshe in #20, so looks appropriate for committing to Drupal 6. Moving to 5.x.

Committed to 5.x