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

Support Unix domain socket (UDS) server #120

Merged
merged 2 commits into from
Nov 16, 2017
Merged

Conversation

andig
Copy link
Contributor

@andig andig commented Oct 7, 2017


Resolves / closes #25

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Awesome, would love to get this in! 🎉

Can you add the missing tests and documentation in the README file? Also, this should probably be integrated into the main Server class.

Keep it up! 👍

@andig
Copy link
Contributor Author

andig commented Oct 7, 2017

Can you add the missing tests and documentation in the README file? Also, this should probably be integrated into the main Server class.

Done :)

if (strpos($path, '://') === false) {
$path = 'unix://' . $path;
} elseif (substr($path, 0, 7) !== 'unix://') {
throw new \InvalidArgumentException('Given URI "' . $path . '" is InvalidArgumentException');
Copy link
Member

Choose a reason for hiding this comment

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

...is InvalidArgumentException -> ...is invalid

@clue
Copy link
Member

clue commented Oct 7, 2017

Thank you for the quick update!

I don't see any major issues after eyeballing this and would love to get this in :shipit: I'll try to get back to this tomorrow and address some minor outstanding formatting issues myself 👍

README.md Outdated
To listen on a Unix domain socket, the uri MUST be prefixed with the `unix://` scheme:

```php
$server = new Server('unix:///tmp/.sock', $loop);
Copy link
Member

Choose a reason for hiding this comment

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

❤️

}

/**
* @covers React\EventLoop\StreamSelectLoop::tick
Copy link
Member

Choose a reason for hiding this comment

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

Why does this test covers React\EventLoop\StreamSelectLoop::tick?

}

/**
* @covers React\EventLoop\StreamSelectLoop::tick
Copy link
Member

Choose a reason for hiding this comment

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

Why does this test covers React\EventLoop\StreamSelectLoop::tick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I must admit that the tests are not original art but all copied from TcpServer- so that's a copy/paste artefact. Remove the covers hint here?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah noticed that, but to be honest that is alright. There are few distinct differences that I'm not bothered by it 😄 . The hints are fine as long as they make sense, covering a loop tick in a server test doesn't =D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I change the TcpServerTest, too?

Copy link
Member

Choose a reason for hiding this comment

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

Nah out of scope for this PR


private function getRandomSocketUri()
{
return "unix://" . sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid(rand(), true) . '.sock';
Copy link
Member

Choose a reason for hiding this comment

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

Just nitpicking here but why not go with uniqid('reactphp-socket-tests-unix-server-', true) to make it explicit when debugging that it's a socket for the UnixServer test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turned out to not be a good idea, see 9993763. I've added a check for the client as well.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, what happened?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Warning is shown (in the tests without @) that the path is truncated. My thinking was that we should rather be explicit with failure instead of the user potentially reusing an existing socket due to same name.

@@ -31,8 +31,9 @@ public function connect($path)
return Promise\reject(new InvalidArgumentException('Given URI "' . $path . '" is invalid'));
}

if (strlen($path) > 104) {
return Promise\reject(new InvalidArgumentException('Given URI "' . $path . '" exceeds maximum allowed length of 104 bytes'));
$maxLen = PHP_OS === 'Darwin' ? 104 : 108;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the max-length including or excluding the trailing NUL byte? A link would probably be good for future reference.

// string > 104 characters
$path = "unix://" . sys_get_temp_dir() . DIRECTORY_SEPARATOR . str_repeat('_', 100) . '.sock';
// string > 104/108 characters
$path = "unix://" . sys_get_temp_dir() . DIRECTORY_SEPARATOR . str_repeat('_', 104) . '.sock';
Copy link
Contributor

Choose a reason for hiding this comment

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

It should probably subtract the length of the sys_get_temp_dir() for the str_repeat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It just needs to be long enough to trigger the exception? I did not want to go into complex scenarios here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but it should trigger on exactly X byte and succeed otherwise, no? Probably should have a tests that it works with one less character.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the discussion so far! I wonder if we really need this check in the first place? It looks like stream_socket_server() rejects an invalid path anyway, so why not rely on this?

https://3v4l.org/Gff8D

This would allow us to remove any platform specific constants / assumptions. What do you think about this?

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, it looks like it actually still succeeds despite this warning and instead creates a truncated socket path. Sorry for the confusion, go ahead :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, your test is invalid, see https://3v4l.org/4OPt0

@@ -65,12 +65,28 @@ public function testValid()

public function testConnectWithOverlyLongAddress()
{
https://serverfault.com/questions/641347/check-if-a-path-exceeds-maximum-for-unix-domain-socket
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing //.

@the-eater
Copy link
Contributor

the-eater commented Oct 11, 2017

What are currently the blocking issues with this PR? I need it in my project and would like to get rid of the dev-unix as v0.8.4, also wouldn't it be sane to stop supporting PHP5.3-5.5 as they are all EOL?

@andig
Copy link
Contributor Author

andig commented Oct 11, 2017

  • I will remove the loop->tick from the tests, then it should imho be ready for final review

Ready for squashed commit?

@WyriHaximus
Copy link
Member

@andig is there a twitter handle or email address we can reach? If you don't want to post it publicly feel free to send an email to the email address on my profile.

$server = new SecureServer($server, $loop, $context['tls']);
if ($scheme === 'tls') {
$server = new SecureServer($server, $loop, $context['tls']);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe export the Server constructing logic to a separate method like this: the-eater@2defb1b?diff=split

Copy link
Member

Choose a reason for hiding this comment

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

Functionally LGTM, however I don't currently see this covered in the test suite (plus minor CS issue above). Can you update this? :shipit: 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@clue what's your expectation here? Testing the type of the create private server member? Would need a hint how to do this due to the private nature.

Copy link
Member

Choose a reason for hiding this comment

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

This class is a facade that mostly contains some glue code to make the underlying APIs more easily accessible. Just check the other high-level functional tests and the code coverage. IMHO it's sufficient to create an Unix domain socket server with this facade and check a client connection can be established.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. Tests added.

@andig
Copy link
Contributor Author

andig commented Oct 19, 2017

Ping @WyriHaximus what is missing to get this aging pr merged? Final comment above?

@WyriHaximus
Copy link
Member

@andig I will have a look this weekend but last time I ran into some issues. Will ping you with some questions then 👍

@clue
Copy link
Member

clue commented Oct 20, 2017

Thank you @andig! We're currently trying to focus our efforts on stabilizing things so we can get a stable v1.0.0 release out as soon as possible. This features was originally planned for v1.1.0 via #25, but given the high quality of this PR I'm open to getting this in before the v1.0.0 release. :shipit:

I would like to get the outstanding v0.8.5 bug fix release out in the next days first before introducing this new feature. Does it make sense for you to postpone this to the next v0.8.6 release so we can look into this immediately after the bug fix release is out?

@andig
Copy link
Contributor Author

andig commented Oct 20, 2017

@clue I was hoping to get this into 0.8.5. It is a new feature but does not break compatibility. Only exception is the handling of maximum UDS path lengths that would otherwise potentially have caused unexpected behaviour. So- the sooner the better as ppm depends on it.


$maxLen = PHP_OS === 'Darwin' ? 104 : 108;
if (strlen($path) > $maxLen) {
return Promise\reject(new InvalidArgumentException('Given URI "' . $path . '" exceeds maximum allowed length'));
Copy link
Member

Choose a reason for hiding this comment

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

I think you're addressing a relevant issue here, thank you! However, I would both get a better understanding of this problem before getting this in and also believe that this is unrelated to this new feature. Does it make sense to you to remove this for now and file this a separate PR? 👍

Some thoughts (short brain dump only):

  • The path limit seems to apply to the path as given, a prior chdir() may be used to work around this for use use cases
  • The unix:// scheme is not included in this limit
  • The limit is system dependent and 108 for Linux and 104 for BSD (Mac) and lower for other systems
  • Linux man pages suggest 108 is a valid length, but PHP already raises a notice here
  • The E_NOTICE was apparently introduced via https://bugs.php.net/bug.php?id=60106 in PHP 5.4.1+
  • All relevant versions (including < PHP 5.4.1) already (silently) truncated the path
  • My experiments: https://3v4l.org/hSRCU

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the experiments :)

The path limit seems to apply to the path as given, a prior chdir() may be used to work around this for use use cases

Would need to double-check.

The unix:// scheme is not included in this limit

Why do you think so? The previous unit tests seemed to indicate that it does?

The limit is system dependent and 108 for Linux and 104 for BSD (Mac) and lower for other systems

Unless we could identify from some setting which those are we could only live with assumptions. Custom kernels might have yet other limits?

Linux man pages suggest 108 is a valid length, but PHP already raises a notice here

I can only imagine PHP takes the trailing 0 into account for that check. Might be a bug?

All relevant versions (including < PHP 5.4.1) already (silently) truncated the path

Meaning your suggestion would be to accept this situation?

I'd be happy to not re-raise this PR but thought it would be good as a precaution as I've only accidentally hit this limit and it might have gone unnoticed.

Long story short: do you think it is worth pursuing this or rather entirely skip it?

@WyriHaximus
Copy link
Member

@andig could you share your nginx config? And how do you deal with existing .sock files when you haven't started the process yet?

@andig
Copy link
Contributor Author

andig commented Oct 22, 2017

@clue thank you for the review. I have removed all path length stuff and will raise WIP PR once this is in and I can rebase on top of it.

could you share your nginx config?

@WyriHaximus I can't- no nginx in use, sorry.

And how do you deal with existing .sock files when you haven't started the process yet?

Not sure. If the socket is not in use the server will allow this. What is the expected behaviour?

@clue clue added this to the v0.8.6 milestone Oct 23, 2017
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for keeping up with this @andig, much appreciated! 👍 I've squashed the 16 commits into 2 commits and made some minor adjustments to documentation, otherwise LGTM, now let's get this in! :shipit: 🎉

@clue clue changed the title Add unix server Support Unix domain socket (UDS) server Nov 13, 2017
@andig
Copy link
Contributor Author

andig commented Nov 13, 2017

Will check the test failures, but those are unrelated.

@andig
Copy link
Contributor Author

andig commented Nov 13, 2017

Should the OSX test failures re-appear please check travis-ci/travis-ci#8552 (comment)

@clue
Copy link
Member

clue commented Nov 13, 2017

Unrelated Mac OS X test failure has been resolved by restarting build, see also #134 for follow-up PR :shipit:

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

🚢 🇮🇹

@WyriHaximus WyriHaximus merged commit 626bf5b into reactphp:master Nov 16, 2017
@andig andig deleted the unix branch November 17, 2017 09:56
@andig
Copy link
Contributor Author

andig commented Nov 17, 2017

@clue is this something that could get packaged into 0.8.6 soonish?

@clue
Copy link
Member

clue commented Nov 17, 2017

@andig Very soon-ish, stay tuned :shipit:

@cboden
Copy link
Member

cboden commented Nov 17, 2017

Thanks @andig! Fantastic work!!! 🎉

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.

7 participants