-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Blank screen served occasionally #10
Comments
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>
.
|
@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. |
- Update composer dependencies - Update test cases for updated phpunit - Add more logging - Add default ttl for SimpleCache::set() method call - Implement integration test suite (guzzlehttp + symfony/process + built-in PHP web-server) - Add integration test for internal PSR-16 filesystem-based adapter (tests are failing) - Add PHP7.1 and PHP7.2 to Travis CI config - Issue-related tests added to group "issue10" and can be selected by "--group issue10" - Some tests are failing due to unpredictable error (possible race condition)
@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. |
My bad, I had not made |
- Update composer dependencies - Update test cases for updated phpunit - Add more logging - Add default ttl for SimpleCache::set() method call - Implement integration test suite (guzzlehttp + symfony/process + built-in PHP web-server) - Add integration test for internal PSR-16 filesystem-based adapter (tests are failing) - Add PHP7.1 and PHP7.2 to Travis CI config - Issue-related tests added to group "issue10" and can be selected by "--group issue10"
- Pull upstream updates - Update code to pass PSR-16 compliance tests - Implement TTL logic in default PSR-16 filesystem-based adapter - Update tests` annotations for better phpunit report
@mmamedov had you seen my updates? |
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:
If you format as follow, you will need to change it when you introduce a longer variable:
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. Thanks! |
@mmamedov sorry fo these unwanted changes with spacing, they occur upon built-in |
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>
.
|
Hmm, I need to spend some time for configuring PhpStorm, thanks for pointing me in this direction. |
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 |
* cs fixes + update composer.json (add psr/log to reguired and monolog/monolog to suggested dependencies) * fix .gitkeep issues marked by .gitignore PhpStorm plugin (replace non-standard .gitkeep with standard .gitignore) * Implement integration test suite for #10 - Update composer dependencies - Update test cases for updated phpunit - Add more logging - Add default ttl for SimpleCache::set() method call - Implement integration test suite (guzzlehttp + symfony/process + built-in PHP web-server) - Add integration test for internal PSR-16 filesystem-based adapter (tests are failing) - Add PHP7.1 and PHP7.2 to Travis CI config - Issue-related tests added to group "issue10" and can be selected by "--group issue10" * Increase default expiration time to 60 seconds * Updates for issue #10 - Pull upstream updates - Update code to pass PSR-16 compliance tests - Implement TTL logic in default PSR-16 filesystem-based adapter - Update tests` annotations for better phpunit report * Fix random_int issue int IntegrationWebServerTest * Fix path to router script for built-in php web-server in IntegrationWebServerTest * Replace random_int() call with rand() in IntegrationWebServerTest for PHP5.6 compatibility * Revert tests with exceptions to upstream version for PHP5.6 compatibility * Fix CS issues
Let's keep this issue open for now, until we are sure it is not the case any more. |
@mmamedov I have an idea for debugging and fixing this issue. There are some cases when What do you think about that? |
I don't think we are using But it's a valid point. I noticed we never check for generated content length in I am going to post a quick fix now and test this. Feel free to review/test. |
@mmamedov I think that PHP uses just the same internal codebase for fetching buffer data in |
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. |
@mmamedov yep, we may add check for min length here and remove it from cache adapter so it will become more generic. |
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. |
@mmamedov the PSR-16 spec have nothing about 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? |
I read PSR 16, it says : 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. PS: Please let's keep commits small. |
@mmamedov any updates on this issue? I am also getting blank pages for some cached urls sometimes only and not able to debug it. |
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:
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.
The text was updated successfully, but these errors were encountered: