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

Cannot use HttpServer with UDS #231

Closed
andig opened this issue Oct 3, 2017 · 9 comments
Closed

Cannot use HttpServer with UDS #231

andig opened this issue Oct 3, 2017 · 9 comments

Comments

@andig
Copy link
Contributor

andig commented Oct 3, 2017

PHP-PM uses UDS for handling master requests on slave. This fails in the RequestHeaderParser:

parser->error Symfony\Component\Debug\Exception\ContextErrorException: Notice: Undefined index: port in /Users/andig/Documents/htdocs/vz/vendor/react/http/src/RequestHeaderParser.php:107
Stack trace:
#0 /Users/andig/Documents/htdocs/vz/vendor/react/http/src/RequestHeaderParser.php(59): React\Http\RequestHeaderParser->parseRequest('GET /data.json ...')
#1 /Users/andig/Documents/htdocs/vz/vendor/react/http/src/RequestHeaderParser.php(49): React\Http\RequestHeaderParser->parseAndEmitRequest(132)
#2 [internal function]: React\Http\RequestHeaderParser->feed('GET /data.json ...')

ping @WyriHaximus can this constraint be removed to allow any type of underlying socket?

@andig
Copy link
Contributor Author

andig commented Oct 3, 2017

Note: for my testing purpose solved by:

    if ($this->remoteSocketUri !== null) {
        $remoteAddress = parse_url($this->remoteSocketUri);

        if (isset($remoteAddress['port'])) {
            $serverParams['REMOTE_ADDR'] = $remoteAddress['host'];
            $serverParams['REMOTE_PORT'] = $remoteAddress['port'];
        }
    }

Host is set to ________ in my tests, so testing for host does not work.

I've also noticed that the Http\Server returned HTTP 400 in this case. Wouldn't it be more appropriate to return HTTP 500 in case of unclear or unexpected (internal) exceptions?

@clue
Copy link
Member

clue commented Oct 3, 2017

Thanks for filing this! This is an interesting issue, given that the underlying component does not currently support Unix domain sockets (UDS) (reactphp/socket#25) :-)

I agree that despite this, this is still a minor bug that parsing the request should probably not fail in this case. Do you feel like filing a PR to fix (and test) this? :shipit:

Considering long-term goals, does it make sense to ignore REMOTE_ADDR and REMOTE_PORT in this case and/or how do other servers handle this?

@andig
Copy link
Contributor Author

andig commented Oct 7, 2017

@clue I've added a test. The problem at least on OSX seems to be related to a PHP bug with UDS that I've seen before (https://bugs.php.net/bug.php?id=74556).

@clue
Copy link
Member

clue commented Oct 7, 2017

@andig Thanks for the update, I've just commented on this and linked to a related ticket (#234 (comment)) 👍

I wonder where this address you're seeing comes from, as this should already be handled by our socket component (reactphp/socket#100). Do you happen to use an older version or a custom implementation that does not implement the same fix?

@andig
Copy link
Contributor Author

andig commented Oct 7, 2017

@clue I think the problem is that I'm using the unix socket on the server side, so the UdsConnector doesn't even come into play (its what you've pointed out via reactphp/socket#25).

@clue
Copy link
Member

clue commented Oct 7, 2017

Thanks for the confirmation! As per reactphp/socket#25 there's currently no official UnixServer implementation (PRs welcome!).

If you're using a custom implementation, I would suggest re-using the existing logic to return semantically correct unix address strings. You should be able to re-use the logic from the above link and/or manually return null in this case. Can you give this a try and see if this fixes your issue?

As an alternative, once reactphp/socket#25 is resolves (PRs welcome!), we can add an integration test for this and make sure this works out of the box 👍

@andig
Copy link
Contributor Author

andig commented Oct 7, 2017

I've opened reactphp/socket#120, currently work in progress. This fixes the problem according to my testing.

Shall we close this PR hence and focus on the proper UDS server? Big thank you for your patience and help!

@clue
Copy link
Member

clue commented Oct 7, 2017

Nice! Also happy to hear this appears to fix the issue you're seeing! I think both ways make sense, so I'll leave the decision up to you. Either way, thank you for your continuous effort! :-)

@andig andig closed this as completed Oct 7, 2017
@andig
Copy link
Contributor Author

andig commented Oct 7, 2017

tl;dr

Still needs reactphp/socket#123 to get in for Mac-specific failure on current PHP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants