The built in cron runner just runs at the end of the request, which is horrible. See #1554872: "Run cron every" setting (cron_safe_threshold) is not an interval but a Boolean for amusing things you can run into now due to this.

It was originally put in running from a 1px gif, but this was reverted later since that 1px gif meant a full Drupal bootstrap every time it was requested (including when that 1px gif was served from a cached page, so you'd hit Drupal even if you were serving pages from varnish).

However there's new tricks now, so we don't need to do either. If we have this, then it'll also be most of the necessary infrastructure for #1189464: Add a 'poor mans queue runner' to core.

This depends on #1447736: Adopt Guzzle library to replace drupal_http_request() since one of the criteria for finding a new http client is that it can handle non-blocking HTTP requests. So marking postponed, but leaving at major since this is a nasty gotcha on sites that don't want it, and crappy performance for those users who are unlucky enough to be the ones to trigger the cron run.

We could also fire each cron hook in its own separate PHP runner to keep memory usage down and prevent an error in one cron job killing the others, doesn't necessarily need to be done here though.

Files: 
CommentFileSizeAuthor
#15 cron-async-request-1599622-15.patch23.89 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 50,741 pass(es).
[ View ]
#13 cron-async-request-1599622-13.patch26.53 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cron-async-request-1599622-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 cron-async-request-1599622-8.patch12.1 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cron-async-request-1599622-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 cron-async-request-1599622-8-system-only-do-not-test.patch1.47 KBBerdir
#4 cron-async-request-1599622-4.patch11.83 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 49,400 pass(es).
[ View ]

Comments

gielfeldt’s picture

For inspiration, Ultimate Cron already does this for D6+D7 using a non-blocking version of drupal_http_request() via Background Process.

Currently mikeytown2 and I are discussing merging the HTTP Parallel Request & Threading Library with Background Process to produce a cleaner separation and better API for respectively http requests and starting background processes.

I know improving the http request API in D8 is in the works, which among other things would probably include non-blocking request possibilities. Perhaps this task also warrants a library for running arbitrary code in the background?

Regarding the individual cron hooks, an extension of this API is being discussed here: #1442434: Collaboration and Drupal 8

EDIT: forgot the link to the background process discussion #1357652: Merge Projects: Replace Background Process's HTTP client with HTTPRL's.

Berdir’s picture

Status:Postponed» Active

Guzzle has been added to core, so this should now be possible...

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new11.83 KB
PASSED: [[SimpleTest]]: [MySQL] 49,400 pass(es).
[ View ]

Here we go. Seems to be working fine for me. The API is not very intuitive, but works.

Had to add the async plugin, copied the version definition (*) from guzzle itself, but I think this wrong, it seems to do a git clone?

David_Rothstein’s picture

Doesn't this need to acquire a lock to avoid a stampede scenario? (If the site is busy and slow such that HTTP requests are having trouble connecting, then it seems like triggering a new HTTP request off of each one which itself will not connect could be a problem.)

I'm also wondering what happens here on servers that are configured to not be able to make HTTP requests to themselves (see #245990: HTTP request testing unreliable and may be undesirable). As I recall there are reports of 30-second delays in those scenarios, and I assume that would be even before any of this non-blocking stuff kicks in (though I'm not sure).

rbayliss’s picture

Regarding the async plugin, I think you can add that plugin to the request rather than the client. So you'd have something like this:

<?php
$request
= $client->get($url);
$request->addSubscriber($async_plugin);
$request->send();
?>
effulgentsia’s picture

copied the version definition (*) from guzzle itself, but I think this wrong, it seems to do a git clone

See #1834594-28: Update dependencies (Symfony and Twig) follow up fixes for Composer. At some point, there may be a Guzzle release we can use instead, but at least as of a few weeks ago, there wasn't.

Berdir’s picture

StatusFileSize
new1.47 KB
new12.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cron-async-request-1599622-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks for the reviews!

- Yes, adding the subscriber to the request is *much* easier, thanks for the pointer.
- Added a lock around the thing, agreed that this makes sense.
- The whole function should probably be moved into a separate listener (it is only invoked by RequestCloseSubscriber) that gets all the things injected. That is, however, not trivial as "the things" means CacheFactory, KeyValueFactory for state, Lock*, guzzle client and there's the do-not-execute-this-on-ajax-requests @todo in RequestCloseSubscriber. Separate issue/follow-up?

* which is broken as it is missing the shutdown function/releaseAll() part as a opposed to lock() and yes, there is an issue for it.

@effulgentsia: Thanks, that explains it. Looking at https://github.com/guzzle/guzzle/commits/master, it seems that the fixes is included in 3.0.6 and 3.0.7, so it should be save to change to that version in yet another separate issue?

YesCT’s picture

Is this the issue referred to in #8

which is broken as it is missing the shutdown function/releaseAll() part as a opposed to lock() and yes, there is an issue for it.

#1825450: Use lock service in lock()

Follow up for

The whole function should probably be moved into a separate listener (it is only invoked by RequestCloseSubscriber) that gets all the things injected. That is, however, not trivial as "the things" means CacheFactory, KeyValueFactory for state, Lock*, guzzle client and there's the do-not-execute-this-on-ajax-requests @todo in RequestCloseSubscriber. Separate issue/follow-up?

needs clarification/correction on what function is to be moved.
#1883896: Move cron invocation to separate listener

Follow up for

Looking at https://github.com/guzzle/guzzle/commits/master, it seems that the fixes is included in 3.0.6 and 3.0.7, so it should be save to change to that version in yet another separate issue?

#1883904: Update to Guzzle 3.0.7

YesCT’s picture

#8: cron-async-request-1599622-8.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, cron-async-request-1599622-8.patch, failed testing.

YesCT’s picture

oops.
#1875818: Update composer.json to use the latest stable 3.0.x version of Guzzle is the one that should be the follow-up.

closed as a duplicate of that: #1883904: Update to Guzzle 3.0.7

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new26.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cron-async-request-1599622-13.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll, no other chances.

Patch is a bit bigger, due to strange changes in the composer files. I updated my local composer version and now it ordered stuff differently it seems...

Status:Needs review» Needs work

The last submitted patch, cron-async-request-1599622-13.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new23.89 KB
PASSED: [[SimpleTest]]: [MySQL] 50,741 pass(es).
[ View ]

That was a re-roll against an old 8.x version, this should be better.

Damien Tournoud’s picture

Status:Needs review» Needs work
<?php
+      // Only execute the request if we can aquire the lock. If this fails then
+      // another request is already taking care of it and we don't have to
+      // retry.
+      if (lock()->acquire('cron_request')) {
+       
// Execute the asynchronous request. This will return immediately with a
+        // HTTP/1.1 200 OK and X-Guzzle-Async as the singe header.
+        $url = url('cron/' . state()->get('system.cron_key'), array('absolute' => TRUE));
+       
$request = drupal_container()->get('http_default_client')->get($url);
+       
$request->addSubscriber(new AsyncPlugin());
+       
$request->send();
+       
lock()->release('cron_request');
+      }
?>

As implemented, the locking is pointless. ->send() is asynchronous, so this code path is always really fast.

What would make sense is to try to acquire the cron lock, so that we don't try to reschedule a cron run while the cron is actually already running (we update system.cron_last at the end of the run).

Berdir’s picture

Right, that makes sense.

There is still a window between checking that and executing the HTTP request where we could try to start multiple cron multiple time. Wondering if we don't care (any bigger site should not use this anyway) or if we should add another state flag or something like that. The locking system unfortunately doesn't allow us to acquire a lock and keep it for the requested time even if the process is finished.

gielfeldt’s picture

IMO it all comes down to if we can reacquire locks across requests.

There's another lock api thread in progress: http://drupal.org/node/1225404

mikeytown2’s picture

Fixing the locks would be nice. In HTTPRL I hack around core to make the magic happen. As a result of this I also have a cron job to clean up orphaned locks.

catch’s picture

mikeytown2’s picture

What if we would allow for contrib to handle/override call_user_func_array() in _drupal_shutdown_function()
#2246659: Use drupal_register_shutdown_function() in a really unique way OR allow for alt call to call_user_func_array() in _drupal_shutdown_function()

That way a 3rd party module could be called instead of call_user_func_array and run the callback in a new process if configured to do so.

dawehner’s picture

Afaik this issue can be marked as solved?

  • Cron now runs at KernelEvents::terminate() so after the respond is sent
  • Cron.php has a lock, @catch do we need more?
catch’s picture

We have cross-request locking now.

@dawehner if KernelEvents::terminate() runs after the page is actually served I'd be very surprised.

Berdir’s picture

@dawehner if KernelEvents::terminate() runs after the page is actually served I'd be very surprised.

It kind of does, but I don't know what exactly that means to a normal browser.

It is a fact that we had weird test failures in the past, because there was code that did run in terminate() while the parent was already dropping tables, which has to mean that he he got the response.

I guess we need some sort of test to make this explicit, not sure if automating that is possible, or if we can do it manually, e.g. by putting a sleep(10) into a cron hook and then executing a request on the parent, with poormanscron enabled.

Berdir’s picture

Ok, reproduced like this:

1. add a sleep(10) to system_cron() or a similar place that runs then.
2. Delete last cron run timestamp: drush ev "\Drupal::state()->delete('system.cron_last');"
3. Request the frontpage with a browser: It waits 10s, network tab does not show any response before it is done
4. Repeat 2, request the frontpage with curl (make sure that page cache is disabled): The complete HTML output is immediately delivered, then the connection stays open for 10s before the bash input comes back.

Conclusion: This doesn't seem to work they way we intended it to at the moment.

However, based on http://codehackit.blogspot.ch/2011/07/how-to-kill-http-connection-and.html, adding a flush() in index.php after send() is called, has a very interesting effect:
a) The browser starts to build the page immediately
b) Network tab shows headers, but not yet the response
c) The browser is still considering to page as loading
d) After 10 seconds, loading is completed, and only then the javascript is triggered, and e.g. the toolbar pops up.

I'm not sure what's what we want...

To verify if cron was executed, run drush ev "var_dump(\Drupal::state()->get('system.cron_last', 0));"

mikeytown2’s picture

What about http://php.net/fastcgi-finish-request

If that call doesn't do it then the only way I see this working is by using guzzle to do an async request to self to run cron. http://guzzle.readthedocs.org/en/latest/clients.html#asynchronous-requests It's one of the main reasons I pushed for guzzle in D8 #1447736: Adopt Guzzle library to replace drupal_http_request(); this exact situation.

catch’s picture

Yes I've been assuming we'd use guzzle for this issue.

We tried things like running cron in a shutdown function and ob_flush() towards the end of Drupal 7 and didn't get anywhere reliable, Berdir's testing suggests not a lot has changed there.

mikeytown2’s picture

You can try out this function: httprl_background_processing(). I did get it working with some browsers but not all; even before I added fastcgi_finish_request to it some time ago. In httprl it can cut the connection at both ends when executing a function in a new background process; it can also return data back thus allowing for multiple processes running and then combining the results back together. Both modes require non blocking (async) http requests.

Fabianx’s picture

So what about if we do:

a) We use an internal callback to cron via a http request (async) - if we can detect where Drupal lives. If that fails, fall back to b).

AND

b) We use what we have now and see why response does not arrive for cron().

I also had interesting effects with b) with the big pipe implementation.

Especially when the JS is in the footer, those browsers seem to react like this. However if just one JS is in the header, at least chrome did start executing the JS immediately.

My initial implementation of big pipe, was that JS is collected from all assets and added after all fragments had loaded - however then the browser never rendered the page as explained by berdir.

Berdir’s picture

Note that I've already implemented a) a while ago, and it's pretty easy with guzzle, we just need an additional composer package and then it's just a few lines of code. And that seemed to work but was then postponed on locking issues...

We just went back to evaluate if it really is necessary, and it looks like the answer is yes, so we need a re-roll of that patch and then add the locking.

Wim Leers’s picture

@catch also pointed to #566494-60: Running cron in a shutdown function breaks the site, which is related to #31.

klausi’s picture

Note that the current AutomaticCron implementation on kernel.terminate works just fine on FastCGI with Apache + mod proxy fcgi + PHP-FPM for example. The reason is that the Symfony Response class calls fastcgi_finish_request() for us which closes the connection.

So this is only a problem on Apache + mod_php, don't use that on D8 prod sites.

catch’s picture

So I still think we should do this, and we added cross-requested locking for config batch so that bit should be doable.

Fabianx’s picture

The possibility for Drupal to reach itself should be in core - regardless if we do it here or elsewhere.

If its in core, it is properly supported and contrib/ (e.g. ultimate_cron) can rely on it.

If it is not in core, we get the same problems with background_process, ultimate_cron, ... as in D7.

mikeytown2’s picture

Having Drupal reach it's self isn't easy. I've had some success in the httprl module; see _httprl_build_drupal_root() for the hoops I have to jump through.

First step is to find the drupal root dir; this is manly a D6 compatibility thing as the D6 & D7 code are the same. Next is handling basic auth. Then there is a lot of logic to try and figure out what IP address can be used to connect to its self; 127.0.0.1 doesn't always work. http vs https. And finally clean urls.

If this was baked into core that would be very nice. In the latest dev version of httprl I'll try a couple of different variations for the hostname when doing a self connection as well as waiting X ms before closing an async connection; see httprl_install_try_different_settings_checker()

Wim Leers’s picture

Damien Tournoud’s picture

I have to reiterate klausi: So this is only a problem on Apache + mod_php, don't use that on D8 prod sites.. FastCGI is the only decent production infrastructure for PHP.

Fabianx’s picture

#38: That is not true, streaming works nicely with Apache, too.

Maybe the response sending misses a flush() in the end?

I still think non-blocking HTTP request to self is better though ...

geerlingguy’s picture

So this is only a problem on Apache + mod_php, don't use that on D8 prod sites.. FastCGI is the only decent production infrastructure for PHP.

@Damien / @klausi - the problem with that statement is that, by and large, the majority of hosts running Apache are still using mod_php; and probably 90% or more of the guides for "build you own LAMP server" on the Internet (even recent once in DO and Linode's libraries) only mention how to use mod_php. I don't think we can ship with features that don't work on the majority of Apache installs...