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 for UNIX sockets #7392

Closed
vlasky opened this Issue Jul 12, 2016 · 12 comments

Comments

Projects
None yet
6 participants
@vlasky
Contributor

vlasky commented Jul 12, 2016

Meteor should have the ability to listen on a UNIX socket, like most network servers.

This is especially useful to people who are running Meteor apps behind Nginx or another web server which is used for SSL termination and efficiently serving static files.

At the moment, the Meteor tool seems to only support TCP, so when running on localhost, Nginx is forced to pointlessly create TCP connections between itself and the meteor tool (which acts as an HTTP proxy) and then creates even more TCP connections between itself and the meteor app (which by default runs on a randomly assigned port). As a result, this incurs additional overhead and leaves lots of TCP connections in the TIME_WAIT state.

I propose that the meteor tool be extended to support UNIX sockets with the following syntax:

meteor run --port unix:/var/run/meteor.sock

And also with the optional --app-port option which uses a fixed port for the app instead of a randomly-assigned one:

meteor run --port unix:/var/run/meteor.sock --app-port unix:/var/run/meteor-app.sock

@mjmasn

This comment has been minimized.

Contributor

mjmasn commented Jul 12, 2016

Can't speak for Meteor regarding the feature request but you absolutely shouldn't be running meteor behind nginx with the meteor command. The meteor command is for development only. You need to use meteor build to create a app bundle that you can deploy as you would any nodejs app. It's in the guide at https://guide.meteor.com/deployment.html#custom-deployment

@vlasky

This comment has been minimized.

Contributor

vlasky commented Jul 12, 2016

Yes, am well aware - our development server also runs behind Nginx. This ensures we can test and measure the impact of Nginx configuration changes too.

My request for UNIX socket support also applies to the app bundle created with the meteor build command.

There's no reason why all components shouldn't have UNIX socket support.

@abernix abernix changed the title from [feature-request] Support for UNIX sockets to Support for UNIX sockets Jul 12, 2016

@abernix

This comment has been minimized.

Member

abernix commented Jul 12, 2016

Not sure what the popularity of this feature will, but those interested, please +1 the original issue!

Difficulty-wise, for the production side, this might be as easy as a change in this function. But for meteor-tool development, it will be more involved.

@abernix

This comment has been minimized.

Member

abernix commented Jul 12, 2016

Can't believe I'm suggesting this, but as perhaps the hackiest workaround ever, if you want to take advantage of the existing exception in there for Windows, you could use the following for your PORT when you start your app (this will only work in production when starting with node, not with the meteor tool):

PORT=/tmp/\\a\\pipe\\a-meteor.sock node main.js

A quick test with netcat shows that it is indeed working...

$ nc -U \\a\\pipe\\a-meteor.sock
GET / HTTP/1.0

HTTP/1.1 200 OK
...
@laosb

This comment has been minimized.

Collaborator

laosb commented Jul 12, 2016

I think it is not necessary for development?

@tom-adsfund

This comment has been minimized.

tom-adsfund commented Jul 18, 2016

I agree with request. There can be a big performance benefit when using Nginx and unix socket instead of TCP -- thousands more requests/s.

@abernix What is the need for the slashes in your hack?

@abernix

This comment has been minimized.

Member

abernix commented Jul 19, 2016

@tom-adsfund Without inclusion of \\anything\\pipe\\ in the socket filename the hack won't work. 😉

See this code to see how I'm exploiting code that is there already for Windows. This obviously isn't ideal, I'm just offering it as a proof of concept/work-around for those who might be in need of it immediately.

@tom-adsfund

This comment has been minimized.

tom-adsfund commented Jul 19, 2016

@abernix I see, quick thinking. Anyway it should be easy enough to make the changes to make it mainstream, and move Meteor further towards large-scale adoption.

@vlasky

This comment has been minimized.

Contributor

vlasky commented Dec 13, 2016

Let's add support for UNIX sockets to the Meteor roadmap:

https://github.com/meteor/meteor/blob/devel/Roadmap.md

@abernix

This comment has been minimized.

Member

abernix commented Dec 14, 2016

I realize it's a very nice performance benefit for a larger project with more traffic and I'm not trying to downplay the importance, but this seems more like a community contribution task than a Roadmap goal.

I'll even go so far as to marking this pull-requests-encouraged as I'm almost positive a well-written pull-request would be accepted. I'm also willing to review any such PR. I've pretty much pointed out where it would need to be done right now and someone who might already need/use this implementation might be ideal for giving it a go (and testing it).

Is this something you'd be willing to do? See guidelines for feature requests and make sure that any questions are asked and answered here in the issue before submitting a PR, but I say someone in the community should give this a go.

@vlasky

This comment has been minimized.

Contributor

vlasky commented May 17, 2017

For those interested, I have submitted a pull request here: #8702

benjamn added a commit that referenced this issue Sep 6, 2017

Merge pull request #8702 from vlasky/devel
Support for UNIX sockets (#7392)
@hwillson

This comment has been minimized.

Member

hwillson commented Sep 7, 2017

PR #8702 has been merged, so this will be coming in a future Meteor release. Thanks to all involved!

@hwillson hwillson closed this Sep 7, 2017

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