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

Add support for Unix domain sockets (UDS) #17

Closed
wants to merge 3 commits into from
Closed

Add support for Unix domain sockets (UDS) #17

wants to merge 3 commits into from

Conversation

RafaelKa
Copy link

@RafaelKa RafaelKa commented Jun 2, 2015

This change adds support for unix domain socket transport for using unix:// for connections.

This change adds support for unix domain socket transport for using unix:// and udg:// for connections.
@@ -18,14 +18,26 @@ public function __construct(LoopInterface $loop)

public function listen($port, $host = '127.0.0.1')
{
if (strpos($host, ':') !== false) {
$localSocket = '';
if (filter_var($host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4)) {
Copy link

Choose a reason for hiding this comment

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

composer.json should reflect that react requires ext-filter

Copy link
Author

Choose a reason for hiding this comment

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

@staabm
This ext. is since PHP 5.2 enabled by default. http://php.net/manual/en/filter.installation.php
I see many methods from multiple PHP ext.s used in react and no entry in composer.json for them.
What is the difference by ext-filter?

Copy link
Member

Choose a reason for hiding this comment

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

Which extensions does React use that aren't declared?

Copy link
Author

Choose a reason for hiding this comment

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

@cboden f.x. PCRE (for preg_replace) is like Filter enabled per default to.

Is it maybe better to preg_match both instead of using filter?

Copy link

Choose a reason for hiding this comment

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

This ext. is since PHP 5.2 enabled by default

@RafaelKa you neverthelesss should declar it, because a lot of people - especially on production servers - use non-default builds (``--disable-all`) which are stripped down to the very least for performance and security reasons.

Which extensions does React use that aren't declared?

@cboden from within this code-window I would say e.g. ext-pcre

Copy link
Member

Choose a reason for hiding this comment

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

you neverthelesss should declar it

I agree, all dependencies should explicitly be defined (and kept to a minimum for obvious reasons). Though in this case, have you checked PHP's installation instructions? (http://php.net/manual/en/pcre.installation.php and http://php.net/manual/en/filter.installation.php)

Which extensions does React use that aren't declared?

None that I'm aware of. It anybody happens to spot one, please file an issue or even better a PR in the relevant component :-)

Required ext-filter for validating ip v4 and v6 addresses to composer.json added
@clue
Copy link
Member

clue commented Jun 2, 2015

👍 on adding support for Unix domain sockets!

However, I'm not particularly happy with the current interface:

$socket->listen(0, 'unix:///var/tmp/demo.sock');

See #6 for an alternative implementation (with a cleaner API IMHO), which involves a BC break.

@RafaelKa
Copy link
Author

RafaelKa commented Jun 2, 2015

@clue
i thought on this solution to, but i think this is not courteous for community. To many people are using this library, therefore i did this backward compatible patch.
However i would prefer #6 at all ;).

UDG sockets are based on datagrams instead of streams of bytes and already a part of React's datagram component.
@RafaelKa
Copy link
Author

RafaelKa commented Jun 2, 2015

"For UNIX domain sockets, file and directory permissions restrict which processes on the host can open the file, and thus communicate with the server. Therefore, UNIX domain sockets provide an advantage over Internet sockets (to which anyone can connect, unless extra authentication logic is implemented)."

What do you think about setting permissions on uds file at creation time?

1: Is current pull request good as is. Make new pull request after this one is merged
2: push https://gist.github.com/RafaelKa/33856a11023455653af8 in this pull request immediately
significant rows are: 19, 28, 41-43

@clue
Copy link
Member

clue commented Jun 7, 2015

What do you think about setting permissions on uds file at creation time?

I think it makes sense to have this feature. However, I'm not a big fan of the API, so perhaps we can postpone this until we find a better API? The Server implements the ServerInterface, and adding UDS parameters to the global ServerInterface sounds odd IMHO.

@RafaelKa
Copy link
Author

RafaelKa commented Jun 8, 2015

@clue, what do you mean with better API?
is WIP for new API?
becomes this lib new structure?

@RafaelKa
Copy link
Author

You added support for Unix domain sockets (UDS) in reactphp/socket-client, i think currently it's the time to go with UDS on serverside!

Are in this PR some deficits present, which need to be fixed?
Or is this PR for garbage and i can forget about it and try something other again without new dependencies?

@mikegioia
Copy link

@RafaelKa @cboden Is there anything holding up the merge of this PR? I'd love to be able to use this without forking :D 🙏

@RafaelKa
Copy link
Author

i can decide here nothing :-P only cboden can do that, not sure but i think it is on hold, because new dependency(filter) is here. If it is so, we must do the same with regex instead of filter.

@cboden
Copy link
Member

cboden commented Jan 14, 2016

I think I would prefer a separate interface as opposed to string parsing. The string parsing is at a higher level of the chain (conceptually) in that's it's a URI whereas this lib is about socket (lower level) I/O.

I think a helper function somewhere (wrapper lib maybe) would be a better place to do string parsing and call the correlating API of TCP or Unix socket.

Thoughts @clue?

@mikegioia
Copy link

Is it possible to mimic behaviour from Socket-Connection with, say, a UnixServer class (leaving the Server class alone)?

$loop = React\EventLoop\Factory::create();
$socket = new React\Socket\UnixServer( $loop );
$socket->on( 'connection', function ($conn) { ... } );

@marcj
Copy link

marcj commented Mar 22, 2016

Can this PR be merged? What's missing?

@clue
Copy link
Member

clue commented May 20, 2016

Can this PR be merged? What's missing?

Several issues have been raised in this ticket, besides it currently has a merge conflict.

Is it possible to mimic behaviour from Socket-Connection with, say, a UnixServer class (leaving the Server class alone)?

I think this makes sense 👍

However, I'm not particularly happy with the current interface:

$socket->listen(0, 'unix:///var/tmp/demo.sock');

I still think this doesn't make much sense. Ticket #19 suggests an alternative API.

@clue
Copy link
Member

clue commented Oct 10, 2017

I'm closing this for now as it hasn't received any input in a while and there is now a new PR #120 to implement this by @andig. Thank you for your effort and filing this PR! If you feel this PR has been closed prematurely and this problem persists, please come back with more details and we can reopen this 👍

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.

6 participants