slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.98k stars 1.95k forks source link

Content-Length header causing truncated content with New Relic #1690

Closed derekjones closed 8 years ago

derekjones commented 8 years ago

With New Relic, or I would imagine any other daemon/agent that modifies the request the web server sends after it receives output from PHP, the response body is truncated.

Looks like this issue was raised awhile ago but the reporting user never responded with details.

You should be able to reproduce by installing the New Relic PHP agent from the link above and using any renderer. The issue appears to be having the content-size header set by the size of the stream, which is incorrect if the server is modifying any of the output after PHP hands it off.

geggleto commented 8 years ago

There is no way for us to fix it... Slim renders at the end of the application life cycle... if there is another piece of software modifying the content, then its that software's responsibility to update the headers accordingly.

derekjones commented 8 years ago

Thanks for the quick reply @geggleto, and for the great work on Slim in general.

There is no way for us to fix it...

I'm not sure I agree, since PHP can't be sure it's the last link in the chain. That header doesn't have to be sent, or Slim could allow a developer to disable that header.

But I understand it's your prerogative to consider it external, and I'll file an issue with New Relic as well. As this will affect any high-performance app built on Slim that wants to use New Relic or a similar server-side injected real-user monitoring script, that's limiting the framework in my opinion, but that's your call.

For anyone encountering this while searching for the same problem, you can disable New Relic RUM for your Slim app, which of course negates all of the benefits of RUM, but at least allows content to be sent in full:

if (extension_loaded('newrelic'))
{
    newrelic_disable_autorum();
}

or you can extend the \Slim\App class and replace the finalize() method with one that doesn't add that header, which will allow you to keep the server-side injected RUM enabled.

class MyApp Extends \Slim\App {

    protected function finalize(\Psr\Http\Message\ResponseInterface $response)
    {
        // stop PHP sending a Content-Type automatically
        ini_set('default_mimetype', '');

        if ($this->isEmptyResponse($response)) {
            return $response->withoutHeader('Content-Type')->withoutHeader('Content-Length');
        }

        return $response;
    }
}

$app = new MyApp;
geggleto commented 8 years ago

Unfortunately due to HTTP protocol, we cannot delegate that responsibility at our package level. Not all of our clients use NewRelic, and supplying a HTTP Message without a Content-Type or Content-Length results in a malformed message.

I would encourage you to look into this further. I have a very hard time believing that a product like NewRelic modifies the message output... there has to be something else going on. That's just my gut feeling.

mathmarques commented 8 years ago

If Content-Length is your problem, you can solve it using a middleware. Like this:

//Middleware
$mdw = function($req, $res, $next) {
    $res = $next($req, $res);

    return $res->withoutHeader('Content-Length');
};

//Add the middleware to app
$app->add($mdw);
derekjones commented 8 years ago

I have a very hard time believing that a product like NewRelic modifies the message output...

It does, see How does it work?. Its agent injects JavaScript by replacing the closing </body> tag of your server's output. It's rather brute force, but allows it to work on all HTML pages on the server, regardless of the delivery mechanism, and without requiring application modification.

Content-Length is a "should" not a "must" header by spec, and without it my understanding is that the server sends the output in chunks without any noticeably deleterious effect. But as there are workarounds (thanks @mathmarques for the middleware tip) to this niche issue, no worries. Thanks!

derekjones commented 8 years ago

Just a note that the middleware solution does not seem to work. The middleware is executing on the requests way in to the app, but not on the way out, so the App is still setting that header.

mathmarques commented 8 years ago

The Middleware won't work. Analysing the code, I figured out that the Content-Length header is added after all "run" cycle. my bad =/ BUT you can remove the content-length doing this:

$app = new \Slim\App();

$app->get('/', function($request, $response){
    return $response->write("Test");
});

// Take a look HERE! :D
$response = $app->run(true); //Silent mode, wont send the response
$response = $response->withoutHeader("Content-Length"); //Remove the Content-Length

$app->respond($response); //Now we send the response
derekjones commented 8 years ago

Ah, much cleaner than extending the base App class, thanks!

silentworks commented 8 years ago

We will be cleaning up some of these methods in the next few versions. I am going to re-open this as its something we should probably look into.

geggleto commented 8 years ago

I found the offending code...

https://github.com/slimphp/Slim/blob/3.x/Slim/App.php#L546-L549

kgustine commented 8 years ago

I was having a similar issue where the last character of my JSON was missing! The app had been working and all the sudden it wasn't.

I finally pasted the raw JSON into Notepad++ and sure enough, the character count was +1 greater than the content-length header. For some reason, I looked back at the beginning of the string and noticed there was a space before the opening character. I soon realized that someone either left an echo statement active or injected a space into PHPs output buffer unintentionally.

I finally found the culprit by placing an exit; statement at the very beginning of myindex.php page and walking it down until I found the source code with a space before the opening <?php tag.

Problem solved! (mine at least)

Seems like if Slim expects to take total control of the response, it should make sure the output buffer is empty.

        $size = $response->getBody()->getSize();
        if ($size !== null && !$response->hasHeader('Content-Length')) {
            ob_clean(); // <-- Clean the buffer!
            $response = $response->withHeader('Content-Length', (string) $size);
        }

Otherwise, add the buffer length to the content length

        $size = $response->getBody()->getSize() + ob_get_length(); // <-- Add buffer length!
        if ($size !== null && !$response->hasHeader('Content-Length')) {
            $response = $response->withHeader('Content-Length', (string) $size);
        }
akrabat commented 8 years ago

I like the idea of add ob_get_length().

Does anyone have a preference over ob_clean(); vs ob_get_length()?

llvdl commented 8 years ago

A third option would be to throw an error or emit a warning if ob_get_length() is not 0.

As a note: neither options will work for content that is being added after App::finalize() has completed. That content will still be truncated by the browser. I don't know when or how New Relic content is added, so this may not be an issue.

grikdotnet commented 8 years ago

Sending the content-length header without being implicitly asked to do it leading to impossibility to integrate Slim into other application.

kgustine commented 8 years ago

I actually agree with @llvdl. If the output buffer is not clean, slim should throw an error due to the difficulty of troubleshooting the side-effects.

geggleto commented 8 years ago

@grikdotnet you can easily unset the header. you can run Slim without $app->run() and render the Response object how you want with or w/o headers.

grikdotnet commented 8 years ago

@geggleto thanks for the advice. I opened the issue to support other users in case the authors will want to solve it. I will use Symfony microkernel and Lumen, they don't have integration limitations.

geggleto commented 8 years ago

@grikdotnet Yeah we are working on adding an option to omit the Content-Type header.

akrabat commented 8 years ago

You can now disable Slim's automatic setting of the content-length with:

$config = [
    'settings' => [
        'addContentLengthHeader' => false,
    ]
];
$app = new Slim\App($config);