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

ExtUvLoop: Fix reporting connection refused errors #207

Merged
merged 3 commits into from Dec 31, 2019

Conversation

@clue
Copy link
Member

clue commented Dec 28, 2019

This PR fixes reporting "connection refused" on affected installation of ExtUvLoop. I have started with adding relevant tests to the test matrix, fixing the underlying issue and included additional tests to run run this on Windows as well.

Resolves #188
Builds on top of #205
Refs #206 to skip a similar problem for the StreamSelectLoop on Windows only.

@clue clue added this to the v1.1.1 milestone Dec 28, 2019
@clue clue force-pushed the clue-labs:ext-uv-connections branch 2 times, most recently from 515c6d7 to 255aeaa Dec 28, 2019
@clue clue force-pushed the clue-labs:ext-uv-connections branch from 255aeaa to 527c60a Dec 30, 2019
The underlying `epoll_wait()` reports `EPOLLOUT|EPOLLERR|EPOLLHUP` on
the affected file descriptor, which `ext-uv` emits as an error code
`EBADF` with no events attached. We explicitly re-enable all active
events on this error event to invoke the writable listener for this
condition to match other event loop implementations and successfully
detect this as a refused connection attempt. All tests are now green.
@clue clue changed the title [WIP] ExtUvLoop: Fix reporting connection refused ExtUvLoop: Fix reporting connection refused Dec 30, 2019
@clue clue changed the title ExtUvLoop: Fix reporting connection refused ExtUvLoop: Fix reporting connection refused errors Dec 30, 2019
@clue

This comment has been minimized.

Copy link
Member Author

clue commented Dec 30, 2019

It works! 🎉

The underlying epoll_wait() reports EPOLLOUT|EPOLLERR|EPOLLHUP on
the affected file descriptor, which ext-uv emits as an error code
EBADF with no events attached. We explicitly re-enable all active
events on this error event to invoke the writable listener for this
condition to match other event loop implementations and successfully
detect this as a refused connection attempt. All tests are now green.

For future reference, here's how to install this extension in a sample Docker container and all the commands I've used to debug this issue:

docker run -it -v /home/me/workspace:/workspace --workdir=workspace ubuntu:latest bash

apt update
apt-get install -y software-properties-common
add-apt-repository ppa:ondrej/php -y
apt-get install -y libuv1-dev php-pear php-dev strace
pecl install uv-beta
echo "extension=uv" >> "$(php -r 'echo php_ini_loaded_file();')"
php -m

cd /workspace
strace php examples/14-http-client-async.php
php -r 'var_dump((new ReflectionClass("UV"))->getConstants());'
vendor/bin/phpunit

Now let's get this shipped! :shipit:

@clue clue requested review from WyriHaximus and jsor Dec 30, 2019
Copy link
Member

WyriHaximus left a comment

🎉

@jsor
jsor approved these changes Dec 31, 2019
@jsor jsor merged commit e79f422 into reactphp:master Dec 31, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@clue clue deleted the clue-labs:ext-uv-connections branch Dec 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.