Cache Optimization and hardening -- [PP-1] Use assert() instead of exceptions in Cache::merge(Tags|Contexts)

I would suggest getting the patch of the referenced ticket in place before starting work with this since it contains code that aids in writing unit tests for classes which can throw assertions, and allows those assertions to be tested cleanly.

It'd be useful to have a proof of concept patch here to validate the performance improvement though - and that in turn would help tip the balance towards getting assertion support into 8.0.0 as opposed to a minor release.

These patches

https://www.drupal.org/node/2408013#comment-9790657

Implement the goals of this ticket.

At Wim's request, working towards removing validateTags(). I'm going to remove the calls to it one patch at a time since the automated testers online run much faster than my local ones (10 minutes vs. nearly an hour and a half).

First test - the call in /Drupal/Core/Cache/ApcuBackend.php at line 169. Also, Changed Cache:: to static:: in the Cache class itself.

Still can't get local simpletests to work on these patches, even under PHP 5.5, due to previously discovered APCu "feature" (a bug they won't fix). Trying this change, passes PHPUnit.

Drupal\Core\Cache\CacheCollector

Adding the db backend's call in now - passes PHP Unit without adjustement.

Ready to profile. One profile should be the patch as is, the other with assertions turned off (modify the .htaccess file)

Attached is the re-roll of the cache optimizations with the finalized assert tools. I've also hardened the cache library forcing calling classes to obey the interface. (a lot of assert('is_string($cid)') calls). Reassigning this ticket's parent to the assert tools.

One outlying test needed it's context_tags_manager mock updated. There are a couple dozen assert fails from the stronger demands of the new assertions that I need to sort through.

For the above.

Let's see how many assert raises this silences.

The two remaining issues are out of scope for this ticket -- A test that has not been working, and the views module passing bad data to the cache system.

In this patch I've corrected the Alias path test to match current behavior, and then silenced the assertion the views module isn't passing.

Test bot wakeup call.

Forgot the return validity assertion must also be muted.

Moving the failing assertions to the previously created child tickets.

Removing two more assertions that aren't safe until the AliasManager is fixed. They will be placed in that child ticket.

Change in assert patch broke the tear down. All should work now.

::kicks the test bot::

Failing is_object assertion moved to it's own ticket. This one should now be clear.

Yay!! Green at last.

Function name change in parent forcing a re-roll.

Recently added test needs the cacheContextsManager method defined for the assertion.

Almost - The tests of this particular group of assertions include checks to see if the assertions are actually firing, and that requires the PHP 7 shim to be finalized.

https://www.drupal.org/node/2536560

That is a much, much smaller issue though.

It is certainly still worth to proceed here ... I wanted to test how much we gain with that. This is just the review only patch rebased against HEAD. Yeah assertions!

Can you describe the test scenario? I'd expected the difference to be bigger.

This was frontpage anonymous without page cache but I would suggest to have some actual stats instead.

I hope this fixes some of the problems.

Pieces of the old assertion patch's traits where being referenced from one of the tests. I think this will run now. Three patches - one is the actual patch, then an interdiff, finally a binding of this patch with #2536560: Runtime Assertion unit and functional testing so that the unit tests can run correctly (some of the tests need to catch \AssertionError )

Rebuild of this patch since so much of the code around it changed.

I'm beginning to think this new test environment has assert disabled. Rerolling against current and trying again as I cannot duplicate errors locally.

K, I'm going to do these one at a time - starting with just Cache::validateTags(). Wim's code review taken into account on this patch.

Test bots are misconfigured.

http://cgit.drupalcode.org/drupalci_testbot/tree/containers/web/web-5.5/...

Current settings:

[Assertion]
; Assert(expr); active by default.
; http://php.net/assert.active
;assert.active = On

; Issue a PHP warning for each failed assertion.
; http://php.net/assert.warning
;assert.warning = On

; Don't bail out by default.
; http://php.net/assert.bail
;assert.bail = Off

; User-function to be called if an assertion fails.
; http://php.net/assert.callback
;assert.callback = 0

; Eval the expression with current error_reporting().  Set to true if you want
; error_reporting(0) around the eval().
; http://php.net/assert.quiet-eval
;assert.quiet_eval = 0

The semicolon before assert.active needs to be dropped. This explains why the tests are running locally but not online.

I looked on my local drupalci testbot after running the Cache test group.

I then exec'd into the web container and looked at the output of phpinfo. It looks like assert is active in the configuration by default. I'll ping someone in #drupal-testing.

php -i -c /etc/php5/cli/php.ini:

assert.active => 1 => 1
assert.bail => 0 => 0
assert.callback => no value => no value
assert.quiet_eval => 0 => 0
assert.warning => 1 => 1

Here's my sqlite file from the Cache testgroup run, which corresponds to the production test bots.

Uploaded phpinfo.txt from the container as well.

Edit: that php.ini file i posted in irc was from /etc/php5/cli/php.ini which was wrong according to this phpinfo output. Sorry.

Forcing assertions to be on (thankfully it's a runtime setting). If this works then the container setting is known to be the culprit.

I tested your patch locally on the testbot, and I still see fails in the Cache group. I think that assertions are active as proven by the phpinfo output I provided, or there is some other hidden setting that needs to be enabled that we are not thinking of.

Attached tarball of my build artifacts from that test run as well.

• DCI_Fetch=https://www.drupal.org/files/issues/remove-cache-validatetags--2454649-7...,.
• DCI_Patch=remove-cache-validatetags--2454649-73.diff,.

Test summary
------------

Drupal\system\Tests\Cache\BackendChainUnitTest               147 passes   2 fails   6 exceptions             
Drupal\system\Tests\Cache\ApcuBackendUnitTest                147 passes   2 fails  13 exceptions             
Drupal\system\Tests\Cache\DatabaseBackendTagTest               8 passes                                      
Drupal\page_cache\Tests\PageCacheTagsIntegrationTest          22 passes                                      
Drupal\system\Tests\Cache\ClearTest                           11 passes                                      
Drupal\system\Tests\Cache\ChainedFastBackendUnitTest         147 passes   2 fails  13 exceptions             
Drupal\system\Tests\Cache\MemoryBackendUnitTest              147 passes   2 fails   2 exceptions             
Drupal\system\Tests\Cache\PhpBackendUnitTest                 147 passes             2 exceptions             
Drupal\system\Tests\Cache\DatabaseBackendUnitTest            160 passes   2 fails  13 exceptions             

Test run duration: 2 min 1 sec

Edit: Yep, just checked the test run and it's still failing.

+++ b/core/lib/Drupal/Core/Cache/ApcuBackend.php
@@ -166,7 +168,7 @@ protected function prepareItem($cache, $allow_invalid) {
+    assert(Inspector::assertAllStrings($tags), 'Cache Tags must be strings.');

The set function cannot continue if tags has strings. The code is continuing on

PHP on the test runner is configured to **not** bail when assert fails, and that is the default behavior for PHP.

+++ b/core/modules/system/src/Tests/Cache/GenericCacheBackendUnitTestBase.php
@@ -218,13 +218,20 @@ public function testSetGet() {
+    $assertion_mode = assert_options(ASSERT_ACTIVE);

So instead, this is not necessary, but the tests should bail if assert is FALSE?

No. We register an assert callback to throw an AssertionError and that should cause a halt. Is the test running catching the exception, logging it and allowing code flow to continue?

I did find one failure to convert validateTags to an assertion - posting that patch. That *might* be the problem. At this point though I'm looking at my own code even harder trying to find the problem.

Still failing... Do you know where in the chain AssertionError is setup? Maybe KernelTestBase doesn't use Drupal\Component\Assertion\Handle and the exception isn't being handled for PHP5.

I added the PHP7 test. Maybe it will pass under there and that could narrow it down.

Yeah, registering the handler would help a lot.

+++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
@@ -232,6 +233,7 @@ protected function setUp() {
+    AssertionHandler::register();

Would it make sense in the grand scheme of using assert() to register the Handler in UnitTestCase? KernelTestBase extends UnitTestCase which extends the generic PHPUnit test class.

Possibly - let's see if this is the problem first then deal with that issue over in it's own ticket. The drupal PHPUnit bootstrap calls to register the assert handler at present and I presumed that file would be called for any PHPUnit test. Is that not so?

Note - double calling the handler register function is harmless, but sloppy.

Now with monkey patching! (aka a "fix" that cannot be the permanent one, but it will shed light on the nature of the problem and might fix it).

Trying with splicing the registry of assertHandle into TestBase.

Ok, pass. Should the code adding the assert handle registry to TestBase be spun out to it's own ticket, or is it fine here?

Ok, fixed the issue above, will appear next patch. Now for cacheContextTag validation.

Ok, now the CacheContextManager::validateTokens

Here we go....

As huge as this looks, it's just adding a mock method with a return of true to cache context mocks wherever they appear in the system.

1. Fixed.
2, 3. The function will accept mixed, but returns FALSE if it isn't an array (first check). Mixed is noted in the @param.
4, 5. Fixed. (Note I forget to hit save and didn't notice until after making the interdiff, so it's missing there. It is in the main patch).
6. This test uses the same data as ::testValidateTags(), which is the previous test. As the docblock says, it's a regression check to insure that the Inspector's assertAllStrings() method will have the same returns as Cache::validateTags() against that data set. I'll admit that it is unusual for the test to be here, but if I moved the test over to the rest of the Inspector tests I'd have to copy over the data provider function as well.
7. I barely understand it. Apparently when writing prophecies you have to state what arguments you should receive in order to pass the test. Each sub array is an argument set that PHPUnit told me it expected but didn't find. It would be cleaner to write it so that any argument would be accepted, but the test is stronger in its current form.

#102.6 - yes let's move things over. It's confusing as written, and just moving code to where it belongs should not be confusing once done, even if the patch size gets a bit bigger.

Sure, let's do that.

RTBC + 1

Committed/pushed to 8.0.x, thanks!

+1 dawehner. Thanks everyone.