Blank screen served occasionally

[spotman]

Hi, @mmamedov !

After a couple of weeks of testing I may confirm the blank screen issue in my production environment. I have no enough data for fixing it so the better way is to create a test case for this issue and collect more debug info (coz production system is under high load and debugging is not reliable). As described in #9 my suggestion is to implement standalone test suite based on:

• page-cache
• monolog
• php internal web server for serving test page and HTTP client for fetching it.
  I think that we need to emulate concurrent requests to the page-cache and check response for blank screen. We may randomize content and headers so different conditions would be tested. We may check different PSR-16 implementations (biult-in and, for example, filesystem-based Symfony cache). Also there is a third-party package for testing any PSR-16 implementation so we may check is built-in one is processing correctly.

If you agree with this way, I may create this test suite in a separate branch and make a PR.

[mmamedov]

Sounds good Denis. Thanks! I will also try to figire this out.

…

On Fri 9. Feb 2018 at 12:00 AM, Denis Terekhov ***@***.***> wrote: Hi, @mmamedov <https://github.com/mmamedov> ! After a couple of weeks of testing I may confirm the blank screen issue in my production environment. I have no enough data for fixing it so the better way is to create a test case for this issue and collect more debug info (coz production system is under high load and debugging is not reliable). As described in #9 <#9> my suggestion is to implement standalone test suite based on: - page-cache - monolog - php internal web server for serving test page and HTTP client for fetching it. I think that we need to emulate concurrent requests to the page-cache and check response for blank screen. We may randomize content and headers so different conditions would be tested. We may check different PSR-16 implementations (biult-in and, for example, filesystem-based Symfony cache). Also there is a third-party package for testing any PSR-16 implementation so we may check is built-in one is processing correctly. If you agree with this way, I may create this test suite in a separate branch and make a PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADnp5fBmApLk9JwogqMjKB2cb1YPdo2pks5tS9ECgaJpZM4R_a6x> .

[spotman]

@mmamedov I've created a simplified test case with default configuration and concurrency but had not seen the blank page yet. May I know which logger/cache/strategy implementation you are using in your project? It will point us to the right direction.

[spotman]

@mmamedov I'd commited my current progress, please read commit message for more info. I will be away for couple of days and will continue to work on this issue right after return.
It looks like the race condition coz sometimes tests are passed and sometimes not. They are failing on the different assertions but the root problem is cache missing. I've changed default expiration time to 60 seconds and now issue-related tests are passing. So I think the problem is fixed coz there are a lot of tests covering headers, response statuses and length and all of them are passing right now. The only thing we need to update is the FileSystemCacheAdapter coz it is not fully PSR-16 compliant. I may do that on the next week.

[spotman]

My bad, I had not made git pull before, I'll merge your master into my issue10 branch and fix conflicts.

[spotman]

@mmamedov I've created a PR #11 so please review it and run checks in your dev environment (I'm using phpunit 6+ but running tests under phpunit 5+ seems to be OK).

[spotman]

@mmamedov had you seen my updates?

[mmamedov]

Hi @spotman! Thanks for the updates, great job!

There about 5k new lines :) I started reviewing, but will also need to check out this on my machine and test this.

One question - were you able to spot the issue with blank pages, and were you able to verify that these changes fix it?

First thing I noticed, is that there are quite a few syntax modification with spaces - I suggest we don't do this. In the long term this will result in many unnecessary files changes, as we add more variables to the code.

Example:

// Preferred way
$key = $this->getCurrentKey();
$this->currentItem = $this->getItemStorage()->get($key);

If you format as follow, you will need to change it when you introduce a longer variable:

// Not preferred, each time a new variable assignment is introduced you need to reformat whole block.
$key                          = $this->getCurrentKey();
$this->currentItem = $this->getItemStorage()->get($key);

Also a few other non trivial spacing changes, etc. This all makes you spend more time on this, and makes it difficult for someone else to review - no one like long code reviews.

Let's try to keep it in smaller increments, relevant to the issue we are trying to solve. Once we have a stable product that we like we could always have a quick overall code cleanup with cosmetic changes.
I think if every fix is this large, it will make it difficult to move faster.

Thanks!

[spotman]

@mmamedov sorry fo these unwanted changes with spacing, they occur upon built-in Reformat code dialog in PhpStorm which I use after every huge code change. I will configure the PhpStorm to not align statements by = symbol.

[mmamedov]

I also use a few plugins for coding standards, but usually when you only reformat your new code it works fine, instead of reformatting whole file you touched.

…

On Sat 3. Mar 2018 at 4:47 PM, Denis Terekhov ***@***.***> wrote: @mmamedov <https://github.com/mmamedov> sorry fo these unwanted changes with spacing, they occur upon built-in Reformat code dialog in PhpStorm which I use after every huge code change. I will configure the PhpStorm to not align statements by = symbol. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADnp5ea6cFo8KcMlVAgrnOv9ii1ENcr6ks5tarsJgaJpZM4R_a6x> .

[spotman]

Hmm, I need to spend some time for configuring PhpStorm, thanks for pointing me in this direction.

[spotman]

One question - were you able to spot the issue with blank pages, and were you able to verify that these changes fix it?

First of all, my primary goal was to create a test suite for producing the blank page issue. I've finished with a test suite which produces a lot of requests with semi-random configuration options and there was no blank page detected. The problem is that I had to slightly modify the original code for a CLI environment so some changes were made simply to make this test suite work. After that I've covered most configuration parameters and had not seen any blank issue. All combinations of the cache providers, loggers and other options does not generate empty page. So the answer is: no, I had not spotted this issue. But the test suite contains checks for a blank page or inconsistent response body. So if the blank page issue still exists then we need to update this test suite and randomize more options. Anyway, the library is passing all the tests and seems to work fine so there is no any risks to deploy this code to production. I will continue debugging the blank issue on my production servers but I also want to use the latest version without patching composer.json on multiple projects and kindly ask you to merge #11 (or help me to add more checks to the test suite by providing your page-cache config).

[mmamedov]

Let's keep this issue open for now, until we are sure it is not the case any more.

[spotman]

@mmamedov I have an idea for debugging and fixing this issue.

There are some cases when ob_get_contents function returns empty string even if it should not. I think that something similar can be the root of this issue in page-cache. I suggest to add check for empty string in ob_get_content call and reject cache item creation in this case (and log an exception for debugging, of course). This will prevent page-cache from creating cached items with empty response and give us a bit more debugging info.

What do you think about that?

[mmamedov]

I don't think we are using ob_get_contents(), we are using ob_start() and rely on PHP's automatic output flushing at the very end of the script (we don't call ob_end_flush anywhere).

But it's a valid point. I noticed we never check for generated content length in storePageContent . CacheItem setContent also doesn't do any validations and allows storing empty content.

I am going to post a quick fix now and test this. Feel free to review/test.

[spotman]

@mmamedov I think that PHP uses just the same internal codebase for fetching buffer data in ob_start / ob_get_contents functions. Anyway, as you said, the additional test for falsy data in ob_start callback would be useful. My suggestion is to create new exception and log it with stack trace so we will understand use-cases of blank screen issue. I'll be happy to review and test this update.

[mmamedov]

Patched a bug when we displayed empty cache page.

We have a min file cache size setting, but we don't respect it when serving cache files.
We might want to fix that, it was probably lost after refactor.

[spotman]

@mmamedov yep, we may add check for min length here and remove it from cache adapter so it will become more generic.

[mmamedov]

Fixed a bug when CacheAdapter was throwing exceptions because of MobileStrategy. We produce a different key with "-mob" at the end for that, and "-" wasn't in allowed characters for Adapter.

@spotman I added tests that would catch error above with invalid cache key. This is another reminder for us to not hurry with refactoring, and invest more in testing and stability.

[spotman]

@mmamedov the PSR-16 spec have nothing about dash symbol and third-party adapters may not support them. My suggestion is to use underscore _ instead of dash - so any other PSR-16 compatible adapter will work.

We are already paying attention to stability, at least me is moving in this direction right now. There are some use cases which are not working in demo scripts but tests for these cases are passing. My commercial projects are using page-cache too and I'm interested in fixing these issues ASAP.

What about min length check? Are you agree with my vision?

[mmamedov]

I read PSR 16, it says : Implementing libraries MAY support additional characters and encodings or longer lengths, but must support at least that minimum.. It also says : The following characters are reserved for future extensions and MUST NOT be supported by implementing libraries: {}()/\@:

From what I read it seems we need to stop enforcing restrictions on keys, except for disabling these characters {}()/\@:.

For min length I don't remember right now from the top of my head, I will need to go through code.
One thing is clear - min_file_size_length I introduced for exact that reason, when it is lower than that size cache should be ignored. Right now seems this is ignored. Feel free to post fix for this, the way you see it suitable.

PS: Please let's keep commits small.

[subhash-evon]

@mmamedov any updates on this issue? I am also getting blank pages for some cached urls sometimes only and not able to debug it.