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

Improve error messages for failed TCP/IP connections without ext-sockets #266

Merged
merged 1 commit into from
Aug 13, 2021

Conversation

clue
Copy link
Member

@clue clue commented Aug 11, 2021

This changeset improves error messages for failed TCP/IP connections when ext-sockets is not available on Linux. This means it will now consistently report errno/errstr on most supported platforms such as this:

$ telnet 192.168.0.1 8080
Trying 192.168.0.1...
telnet: Unable to connect to remote host: Network is unreachable
$ php examples/11-http-client.php 192.168.0.1
RuntimeException: Connection to tcp://192.168.0.1:80 failed: Network is unreachable in …/TcpConnector.php:115

Builds on top of #265, #171, reactphp/dns#171, reactphp/dns#172 and others

@clue clue added this to the v1.10.0 milestone Aug 11, 2021
@clue clue force-pushed the errno branch 5 times, most recently from 7097f5c to e6dc195 Compare August 12, 2021 11:01
@clue
Copy link
Member Author

clue commented Aug 12, 2021

I've updated this to use the above work-around only when applicable. This is only known to work on Linux and known to not work on Mac and Windows. Using ext-sockets to access the SO_ERROR remains the preferred way, so most other platforms should be supported as well.

For future reference: This work-around uses "error slippage" which has been known to work for decades, see e.g. https://cr.yp.to/docs/connect.html. The recommended way would be to use a read(fd, 0) call, but PHP's fread($stream, 0) rejects empty reads and a fread($stream, 1) will return false without exposing the errno/errstr. Using fwrite($stream, $data) is known to work on Linux, but we might be able to find other platforms that could potentially also take advantage of this. Given that this is only used when ext-sockets is not available and this changeset already provides value on its own, I would suggest leaving this up for a future PR.

This is ready for review :shipit:

@clue clue requested a review from WyriHaximus August 12, 2021 12:44
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.

2 participants