Removes Content-Length injection and output buffer flushing #270
Conversation
By removing a call to `ob_start()` within `Server::listen()`, we can solve the issue of detecting when we have both a response and content in the output buffer; in most cases, we will have already sent headers, which will cause an exception to be raised; we can also check the _current_ output buffer and, if non-empty, raise an exception. This means we can: - Remove the `SapiEmitterTrait::flush()` implementation, and all calls to it. - Remove the `$maxBufferLevel` argument to each emitter. - Remove tests regarding interactions of emitters with the output buffer. This is a backwards-incompatible change. However, it fixes a rather tricky problem that occurs currently when mixing buffered output and response instances.
|
Pinging @mindplay-dk, @stuartherbert, and @mnapoli for review. |
Per zendframework#251, this patch removes Content-Length injection from SAPI stream emitters. It also renames the `checkForPreviousOutput()` method to `assertNoPreviousOutput()` to make it more clear that the method is a no-op under proper usage. The `assertNoPreviousOutput()` method also borrows an idea from the comment to the issue, indicating we can check for (1) a non-zero output buffer, and (2) use `ob_get_length()` to determine if it has content.
weierophinney
force-pushed
the
hotfix/251a
branch
from
191699a
to
0725467
Compare
| * @param int $maxBufferLength Maximum output buffering size for each iteration | ||
| */ | ||
| public function emit(ResponseInterface $response, $maxBufferLevel = null, $maxBufferLength = 8192) | ||
| public function emit(ResponseInterface $response, $maxBufferLength = 8192) |
There was a problem hiding this comment.
Ironically, we changed the signature of this method to swap the $maxBufferLength and $maxBufferLevel arguments for 1.5.0, fixing an issue whereby 2 bytes at a time were streamed; had I resolved this issue prior to that release, we would not have needed to make that change, and then revert it here.
| } | ||
|
|
||
| return $response; | ||
| if (ob_get_level() > 0 && ob_get_length() > 0) { |
There was a problem hiding this comment.
@mindplay-dk: note that I separated out this from the headers_sent() check; I did this to allow having discretely different exception messages.
There was a problem hiding this comment.
line 32: the PR is called 'Remove output buffer awareness', but line 32 is checking the output buffer :)
There was a problem hiding this comment.
Technically, so is calling headers_sent(). :)
I suppose I should retitle it "remove output buffer flushing"...
There was a problem hiding this comment.
Re-named the PR to "Removes Content-Length injection and output buffer flushing".
|
|
||
| class ServerIntegrationTest extends TestCase | ||
| { | ||
| public function testPassesBufferLevelToSapiStreamEmitter() |
There was a problem hiding this comment.
This test was added as part of the patch around the argument swap in SapiStreamEmitter, and thus became unnecessary once we stopped working with the output buffer.
|
This looks good to me, the BC break is unfortunate but better to break it now. |
- Ensures they do not discuss output buffering. - Adds section on `SapiStreamEmitter`.
weierophinney
changed the title
weierophinney
changed the title
| { | ||
| if (headers_sent()) { | ||
| throw new RuntimeException('Unable to emit response; headers already sent'); |
There was a problem hiding this comment.
Why was this removed? You won't be able to emit the status line or headers if output has already been started - the first call to header() will error. Isn't it better to have the exception, which explains why?
There was a problem hiding this comment.
Oh, never mind, you just moved it somewhere else ;-)
|
Wait, one thing... and maybe this was already discussed, but I'm a bit fuzzy right now... it's now never injecting the |
|
@mindplay-dk For those cases, I introduced the What we're doing here is simply saying: we won't auto-inject it. We could continue doing that, I suppose, as we're now asserting that the output buffer has not been used before we emit anything. I'd gone with the suggestion @stuartherbert made and removed that auto-injection, as the main reason it had been present previously was due to an assumption that HTTP/1.1 servers would degrade the connection if it was missing, and he discovered that was not the case. The spec indicates that if it is not present, the server should send a I'd be fine with re-adding the auto-injection; my main concern is that it might still lead to issues. As an example, with the current checks, the following may still occur:
In those circumstances, we might still end up with the current situation where we end up with truncated content, which would be difficult to debug. With the proposed solution, we can ask the reporter if they have enabled the Thoughts? |
|
I did some testing on this, the results are here: #251 (comment) Long and short of it: it'll only break non-compliant clients that haven't fully implemented HTTP/1.1 support, if you're using PHP-FPM. That's acceptable to my user base. I can appreciate it might not be acceptable for everyone. |
|
@weierophinney hmm, I think I'm actually OK with the emitter not adding the Since the emitter isn't doing the header anymore, this gives us a bit more control in this regard. I think this is a great improvement. One last remark: I was a bit surprised by this change being made available in a minor release. This is a breaking change - in some cases with potentially problematic issues arising from it, such as (for example) losing the progress bar on large downloads, since the client can no longer know the content length. There were also breaking API changes to Perhaps you're not committed to SEMVER? These days I kind of assume everyone is. If not, I think maybe I need to adjust our version constraint and stop using the |
|
@mindplay-dk
You have to add the new middleware, this ensures backward compatibility. From the release notes:
Right! This is a problem, because both methods are a part of the public API and these are incompatible changes. /cc @weierophinney |
|
@weierophinney |
|
The rationale for releasing this in a new minor version, versus a major version, is that the component was fundamentally broken without the fix; pushing it to a new major version would prevent many developers from adopting the fixed code. It's not something I do lightly, or often. However, when the only way to provide a fix is via a BC break, it's something we consider. In this particular case, I felt the drawbacks would not affect the majority of users. As Stuart's research shows, the initial rationale for auto-injecting the As such, I determined that for the vast majority of users, there is no perceivable BC break. Those affected will be:
The latter is very much an advanced use case. The former is not, but is a minority use case, and, as noted addressed easily. I stand by the decision to release as a minor release. Again, as noted, it was not a decision taken lightly, but in the interest of getting a substantial fix to as many users as possible. |
|
@weierophinney thank you for the clarification. makes sense! I don't know if SEMVER really stipulates anything one way or the other for a "breaking fix" - it's not an everyday thing, I suppose. Good work :-) |
|
It kinda defeats the purpose of semver if the user cannot trust that you wouldn't introduce BC breaks.
The user should be the one making the decision to upgrade.
It could be released as a major version. The old version could be marked deprecated. |
|
@teohhanhui Shipping a bugfix that requires the user to change their dependency constraints to adopt is also a poor user experience. Yes, in a perfect world, we would not ever, ever break backwards compatibility in a bugfix or minor release. Practically speaking, however, sometimes it's a necessary evil. I can count on one hand how many times I've done this in the past 10 years. |
|
@teohhanhui does make a valid point here - I too was surprised that this was released as a non-breaking change. Developers rely on
Shipping a breaking change against the users wishes - per their dependency constraints - requires the user to discover (potentially really subtle) bugs (such as a missing header) on their own, which is a much worse user experience. Having to change your dependency constraint to adopt is SEMVER and Composer package constraints working as intended - it may not be a wonderful experience, but it works that way for good reasons.
|
Per discussion on #251, we have two "features" in Diactoros that tend to cause problems:
Content-Lengthheader causes issues when any output is emitted via the output buffer. What tends to happen is that the contents of the output buffer are emitted up to theContent-Length. As a result, this content is truncated, or, in the case that it is shorter, the original content of the response is truncated. In either situation, the results are difficult to debug.Content-Lengthissues previously mentioned, but also mixed content, which typically breaks clients.As discussed in the issue, the proper solution is a couple of BC breaks:
Content-Lengthauto-injection. If developers want this, they can now useZend\Expressive\Helper\ContentLengthMiddlewarefrom zend-expressive-helpers 4.1.0.Serverclass. Emitters now check for either headers sent, or an existing output buffer with content length> 0; if either case occurs, they raise an exception, as the emitter cannot properly emit the response.Essentially, this approach forces the developer to choose to use PSR-7 fully, or create their own emitter and/or server implementation if they want to allow mixed global/PSR-7 usage.