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

Remove error suppression from stream_select() #1444

Merged
merged 2 commits into from Jan 17, 2020
Merged

Conversation

@tomsommer
Copy link
Contributor

tomsommer commented Jan 10, 2020

Suppressing errors from stream_select() makes debugging timeouts extremely hard.

Suppressing errors from stream_select() makes debugging timeouts extremely hard.
@terrafrost

This comment has been minimized.

Copy link
Member

terrafrost commented Jan 16, 2020

According to https://bugs.php.net/42682 the issue that motivated me to add error suppression has been fixed. I'll try to confirm when I get home and if it is indeed no longer an issue I'll merge it.

Thanks!

@terrafrost

This comment has been minimized.

Copy link
Member

terrafrost commented Jan 17, 2020

So it looks like error suppression (which has been in place since the timeout code was added on Jun 23, 2012) actually preceded the && !count($read) bit (which has been in place since Aug 29, 2012). The https://bugs.php.net/42682 link apparently applies to the && !count($read) bit specifically.

That said, I'm not able to reproduce either issue on Windows. I tested it as low as PHP 5.0.5. It looks like a lot of people had issues reproducing the bug even in the bug report. As such I'm not even sure unit tests against https://www.appveyor.com/ would have helped.

Anyway, can you resubmit this PR but target the 3.0 branch instead? They're pretty much identical so it shouldn't be a ton of effort on your part (whereas on my part I'd have to add you as a remote then I'd have to cherry-pick your commit into the 3.0 branch).

Thanks!

@terrafrost

This comment has been minimized.

Copy link
Member

terrafrost commented Jan 17, 2020

Oh - and while you're at it, might as well remove the && !count($read) bit as well. Just for the 3.0 branch. If that causes the unit tests to fail (which I don't expect) then readd it.

Thanks!

@tomsommer tomsommer changed the base branch from master to 3.0 Jan 17, 2020
@tomsommer

This comment has been minimized.

Copy link
Contributor Author

tomsommer commented Jan 17, 2020

Done

@terrafrost terrafrost merged commit cb87d18 into phpseclib:3.0 Jan 17, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.