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:// transport #78

Closed
wants to merge 6 commits into from
Closed

Add support for unix:// transport #78

wants to merge 6 commits into from

Conversation

s2x
Copy link

@s2x s2x commented Mar 1, 2017

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

@clue
Copy link
Member

clue commented Mar 9, 2017

While this may look good functionally, it unfortunately lacks documentation and tests. I've prepared a very much similar PR, do you feel like updating or should I file mine? 👍

@s2x
Copy link
Author

s2x commented Mar 16, 2017

I added some test, but hhvm fails. I will try to fix it tomorrow.

@s2x
Copy link
Author

s2x commented Mar 16, 2017

Ok, i made ugly hack for hhvm because there is a bug in hhvm (see facebook/hhvm#7733).

Tomorrow I will add some info in readme about the unix sockets.

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.

I'm currently undecided if it makes sense to have this in the Server or if we should move this to a dedicated UnixServer. What do you think?

//if unix socket we must delete socket file
if ($metaData['stream_type'] == 'unix_socket') {
//ugly hhvm hack see bug https://github.com/facebook/hhvm/issues/7733
unlink(rtrim($socket_path,":"));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this workaround only be applied to affected versions?

Copy link
Author

Choose a reason for hiding this comment

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

I will prepare new PR for this

fclose($this->master);

//if unix socket we must delete socket file
Copy link
Member

Choose a reason for hiding this comment

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

I agree that this probably makes sense in most cases, but is this really a "must"?

Copy link
Author

Choose a reason for hiding this comment

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

We need to delete socket file, because the socket file will remain after server close.

clue added a commit to clue-labs/socket that referenced this pull request Mar 29, 2017
Simplify test suite by relying on React's secure TLS server
@s2x s2x closed this May 26, 2017
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.

3 participants