Skip to content
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

Page integrity is not checked when putting buffer to cache file #1883

Closed
leurdo opened this issue Aug 5, 2019 · 5 comments
Closed

Page integrity is not checked when putting buffer to cache file #1883

leurdo opened this issue Aug 5, 2019 · 5 comments
Labels
module: cache type: enhancement Improvements that slightly enhance existing functionality and are fast to implement

Comments

@leurdo
Copy link

leurdo commented Aug 5, 2019

Describe the bug
My site has some 300K pages, it is very old, and without cache page load is very slow. So, I noticed that sometimes pages are not loaded to cache completely. This means that html in cached file is broken (half of page is missing). I think that due to some timeout or when the page load is extremely slow, page is not loaded completely. And this may happen, that such page version can go to cache.
Apart from load speed problems on my site, the problem is, that page integrity in buffer is never checked.

Here is part of your function maybe_process_buffer from class-cache.php

public function maybe_process_buffer( $buffer ) {
		if ( ! $this->tests->can_process_buffer( $buffer ) ) {
			$this->log_last_test_error();
			return $buffer;
		}

		$footprint = '';
		$is_html   = $this->is_html( $buffer );
		
		if ( ! static::can_generate_caching_files() ) {
			// Not allowed to generate cache files.
			if ( $is_html ) {
				$footprint = $this->get_rocket_footprint();
			}

			$this->log(
				'Page not cached by filter.',
				[
					'filter' => 'do_rocket_generate_caching_files',
				]
			);
			return $buffer . $footprint;
		}

		$cache_filepath = $this->get_cache_path();
		$cache_dir_path = dirname( $cache_filepath );

		// Create cache folders.
		rocket_mkdir_p( $cache_dir_path );

		if ( $is_html ) {
			$footprint = $this->get_rocket_footprint( time() );
		}

		// Save the cache file.
		rocket_put_content( $cache_filepath, $buffer . $footprint );
               //... rest of code
}

$is_html reflects html integrity but is not used to stop caching if there is no closing tag.

Expected behavior
You could prevent uncomplete $buffer from caching as following:

public function maybe_process_buffer( $buffer ) {
		if ( ! $this->tests->can_process_buffer( $buffer ) ) {
			$this->log_last_test_error();
			return $buffer;
		}

		$footprint = '';
		$is_html   = $this->is_html( $buffer );

		//TODO: is_html() check fix
                if ( ! $is_html ) {
                     return $buffer;
                }

               //... rest of code
}

I tried this solution on my site, and now I am getting no more broken pages.
Of course, there could be more elegant solution, because you know your code better than I do.

@Tabrisrp
Copy link
Contributor

Tabrisrp commented Aug 5, 2019

That's interesting, I don't think it's something that came up before.

There is a reason we don't stop caching if there is no closing </html> tag, and that is because it's possible to cache the REST API responses with the plugin, which are in JSON format.

But there could still be some additional checks to make sure the whole HTML content is there when caching a normal page.

@Tabrisrp Tabrisrp added module: cache type: enhancement Improvements that slightly enhance existing functionality and are fast to implement labels Aug 5, 2019
@leurdo
Copy link
Author

leurdo commented Aug 5, 2019

@Tabrisrp Now I understand, why there was no such check, thank you.

It woud be nice to have some solution before next update, or I will get my bug back :)

@JeppeKr
Copy link

JeppeKr commented Dec 4, 2019

@arunbasillal This has not been fixed properly, please read on :)

Your recent implemented fix, reports issues of missing ,,wp_footer() to the admin user.
This is perfect for informing about theme issues, prohibiting WP Rocket to function normally.

But the issue @leurdo experienced, which we are also experiencing on multiple customers is much more important.

During a cache load, somehow WP Rocket/WP is interrupted, but WP Rocket still saves the cache.
This provokes the "White page bug", delivered to all visitor until the cache runs out.

We are implementing @leurdo 's fix on every customer to prevent this..
We have been looking into error_logs etc. but are unable to detect the problem in any themes of plugins. The problem has been reported by 5 customers so far.

@arunbasillal
Copy link
Contributor

@JeppeKr Thanks for the feedback. This issue isn't closed yet as you can see. I will look into this and consider this for an upcoming major version :)

@piotrbak
Copy link
Contributor

Hello @JeppeKr and @leurdo We have fixed the issue while ago with this PR:
#3713

I'm closing the issue and if you are still experiencing the problem, please reopen it and let's discuss.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: cache type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

No branches or pull requests

5 participants