Make WordPress Core

Opened 9 years ago

Last modified 5 years ago

#31200 new defect (bug)

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: needs-patch
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 (3)

#1 @Fab1en
9 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
9 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
8 years ago

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