Make WordPress Core

Opened 11 years ago

Closed 12 months ago

Last modified 12 months ago

#31200 closed defect (bug) (invalid)

wp_redirect Missing Body - Causes Performance Issues

Reported by: tripsis's profile tripsis Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.1
Component: General Keywords:
Focuses: performance Cc:

Description

When I changed my site over to a new setup using nginx and Varnish, I noticed some performance issues when doing things like submitting a comment, activating plugins, etc.

I narrowed it down to a problem with wp_redirect not providing a body, and thus nginx does not provide a content length. This causes Varnish to hang, waiting for a body, until it hits a time out (which by default is 5 seconds). This makes any action that involves a redirect take a minimum of 5 seconds (or whatever the time out is set to).

Adding some output to wp_redirect immediately solved the problem for me.

Even though this may relate to a specific nginx/Varnish setup, the HTTP standards also say you should always include some kind of output in the body. Nginx + php-fpm does not do this by default, and given that this is an increasingly common stack it would be useful if it conformed to standards.

The issue can also be sidestepped by adding:

header( "Content-Length: 0" );

However, this does not conform to RFC specs.

See: http://www.ietf.org/rfc/rfc2616.txt under 10.3.2 and 10.3.3

HTTP 301:

The new permanent URI SHOULD be given by the Location field in the
response. Unless the request method was HEAD, the entity of the
response SHOULD contain a short hypertext note with a hyperlink to
the new URI(s).

HTTP 302:

The temporary URI SHOULD be given by the Location field in the
response. Unless the request method was HEAD, the entity of the
response SHOULD contain a short hypertext note with a hyperlink to
the new URI(s).

Change History (6)

#1 @Fab1en
11 years ago

Hi tripsis, and thanks for reporting this issue.

If the issue only happens with Nginx + Varnish configuration, I think the correction should be specific to this configuration. There is perhaps an option in the varnish config to immediately return when a 301 redirect is detected ? Here is a post that describe the settings needed for NGinx + Varnish for WordPress.

Correcting this behavior to comply to RFC specs might raise new issues. wp_redirect does not call exit after having set the redirection status, but rely on the caller to do it, so output some content could break some existing features. For example, when a plugin is activated, a wp_redirect to the failure page is called before activating the plugin, in the case where a fatal error happens during plugin activation. Redirect status is then changed back to success page if the plugin has been activated successfully.

#2 @tripsis
11 years ago

Thanks Fab1en,

It looks like that config you sent me is for Varnish 3. I'm using Varnish 4, which is pretty new but is ultimately gaining in popularity. The config works perfectly fine, except for this one issue, which isn't present in Varnish 3. There doesn't seem to be anything different in the config you sent me in regards to this problem, which is why I presume it's just handled differently in version 4.

Perhaps they're following the RFC spec more closely in Varnish 4, because it's worth noting that this is not a WordPress specific problem. It's reproducible with just the following code:

<?php header("Location: http://google.com"); ?>

However, I would expect to start seeing it become more of a problem as more people move over to Varnish 4.

I understand that you can't output a body in the wp_redirect function because this may break existing plugins, etc., but I'd argue that for redirects in the WP core you should output a body to comply with the RFC spec even if this body is defined outside of the wp_redirect function, in order to not break legacy code.

#3 @swissspidy
10 years ago

  • Keywords needs-patch added

#4 @pbearne
23 months ago

is this still an issue?

#5 @pbearne
12 months ago

  • Resolution set to invalid
  • Status changed from new to closed

I had a look at adding a meta tag redirect to the output

It seems that while well-intentioned (perhaps for very old browsers), this is generally problematic and redundant.

Here's why it's problematic:

  • Redundancy: Modern browsers respect the Location header and perform the redirect immediately. The meta refresh is unnecessary and adds extra processing.
  • SEO: Search engines generally follow the Location header. Adding a meta refresh might confuse them, potentially diluting link juice or causing indexing issues.
  • User Experience: A fast redirect via the Location header is seamless. The meta refresh introduces a slight delay and an unnecessary interstitial page, potentially disrupting the user experience.
  • Interference with Caching: The outputted HTML might be cached, leading to stale redirects.

Recommendation: Remove the added code block entirely. The existing header("Location: ...") is sufficient for proper redirection. The wp_redirect_force_status_header filter seems designed to control this problematic behaviour, but defaulting to true makes the problem the default. A better approach would be to default it to false and only enable it in very specific cases where the Location header is known to be unreliable.

In short, the diff makes WordPress redirects less efficient and potentially harmful. Reverting this change is recommended.

so I am going to close this ticket as won't fix

#6 @pbearne
12 months ago

  • Keywords needs-patch removed
Note: See TracTickets for help on using tickets.