Skip to content
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

Work around write chunk size for TLS streams for PHP < 7.1.4 #114

Merged
merged 1 commit into from
Sep 8, 2017

Conversation

clue
Copy link
Member

@clue clue commented Sep 8, 2017

PHP < 7.1.4 (and PHP < 7.0.18) suffers from a bug when writing big chunks of data over TLS streams at once. We try to work around this by limiting the write chunk size to 8192 bytes for older PHP versions only. This is only a work-around and has a noticable performance penalty on affected versions. Please update your PHP version.

Closes / resolves #105

@clue clue added the bug label Sep 8, 2017
@clue clue added this to the v0.8.3 milestone Sep 8, 2017
// affected versions. Please update your PHP version.
// This applies to all streams because TLS may be enabled later on.
// See https://github.com/reactphp/socket/issues/105
$limitWriteChunks = (PHP_VERSION_ID < 70018 || (PHP_VERSION_ID < 70114 && PHP_VERSION_ID >= 70100));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be PHP_VERSION_ID >= 70100 && PHP_VERSION_ID < 70114 for better readability IMO.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this does not match the same version range. The fix landed in PHP 7.1.14 and PHP 7.0.18, which means that all earlier versions are affected (such as 7.1.13 and 7.0.17, but not 7.0.19)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure, just talking about the second condition being switched.

PHP_VERSION_ID < 70018 || PHP_VERSION_ID >= 70100 && PHP_VERSION_ID < 70114

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think either version is any clearer, so I've just updated this to match your suggestion :shipit: 👍

@clue clue changed the title Work around write chunk size for TLS streams for PHP < 7.1.14 Work around write chunk size for TLS streams for PHP < 7.1.4 Sep 8, 2017
@clue
Copy link
Member Author

clue commented Sep 8, 2017

Wait a moment, typo'd the version, this has been fixed in PHP 7.1.4, not PHP 7.1.14…

@clue
Copy link
Member Author

clue commented Sep 8, 2017

Sorry for the confusion, PR updated to reference affected PHP < 7.1.4 now :shipit:

See also #105 and php/php-src@17e9fc9

@WyriHaximus WyriHaximus merged commit 1005359 into reactphp:master Sep 8, 2017
@WyriHaximus
Copy link
Member

:shipit:

@clue clue deleted the tls-buffer branch September 8, 2017 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when sending large chunks of data over a secure TLS connection with older PHP versions
4 participants