Skip to content

Fix #48725: Support for flushing in zlib stream #3320

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

Closed
wants to merge 2 commits into from

Conversation

aboks
Copy link
Contributor

@aboks aboks commented Jun 23, 2018

Actually this bug is not limited to zlib, but more about stream filters not being called with the CLOSING flag for data-, temp- and memory-streams. The more generic testcase aims to illustrate this.

This issue was fixed in 5.5.21 by 86f1875, but resurfaced in 7.2.0 due to 5060fc2. Apparently the fix for another break in 0a45e8f was not enough to fix this issue.

This PR takes another approach by ensuring that a final pass through the stream filter with the CLOSING-flag is made, even if the stream turns EOF directly after reading the last bytes.

aboks added 2 commits June 23, 2018 17:27
There are two tests: one specifically for the issue applied to the
zlib stream filter, one for the generic issue of stream filters
not being called with the CLOSING flag.
It seems that data streams turn EOF directly after reading the last few bytes,
to that the stream filter is never called with the CLOSING flag. By ensuring
that we make an extra pass through the stream filter if the last read was not
empty, we can guarantee that the stream filter is always called with the
CLOSING flag in the last pass.
@aboks aboks changed the base branch from master to PHP-7.2 June 23, 2018 17:16
@cmb69
Copy link
Member

cmb69 commented Aug 19, 2020

While working on a fix for bug #48725, I've found this PR. Thanks for it! However, bug you have found, and which this PR addresses is not the one bug 48725 is about, but is rather bug 79984. It seems to me, though, that bug 79984 and bug 77069 have the same root cause, but the latter issue would not be fixed with this PR, but rather with PR #6001. However, PR #6001 currently causes a SOAP test to fail, which this PR would not. I'm still looking for a solution; maybe you have an idea?

@nikic
Copy link
Member

nikic commented Mar 16, 2021

@cmb69 Can this one be closed after the changes you have done?

@cmb69
Copy link
Member

cmb69 commented Mar 16, 2021

Ah, good catch, this can indeed be closed now.

@cmb69 cmb69 closed this Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants