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) #8702
Conversation
…treated as the UNIX domain socket file to listen for connections on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this PR @vlasky! I've looked things over and added a few comments, but this will be reviewed more thoroughly very shortly. Thanks again!
packages/webapp/webapp_server.js
Outdated
WebAppInternals.parsePort = function (port) { | ||
if( /\\\\?.+\\pipe\\?.+/.test(port) ) { | ||
if( /\\\\?.+\\pipe\\?.+/.test(port) || /^(.+)\/([^/]+)$/.test(port) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the existing parsePort
tests to verify this additional parsing works properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done :-)
packages/webapp/webapp_server.js
Outdated
httpServer.listen(localPort, localIp, Meteor.bindEnvironment(function() { | ||
|
||
if (typeof localPort == "number") | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a nit pick, but can you follow the existing opening/closing brace style that's used in the rest of the file (open brace on same line, etc.)?
packages/webapp/webapp_server.js
Outdated
console.log("Deleting stale socket file"); | ||
fs.unlinkSync(socketPath); | ||
|
||
httpServer.listen(listenOptions, Meteor.bindEnvironment(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this httpServer.listen
block of code is repeated a few lines down, and is the exact same. Could this be extracted into a common function to clean the code up a bit?
…cketPath and made cosmetic fixes to UNIX socket code to conform better with meteor's coding style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this PR! It would certainly be great to support Unix sockets.
packages/webapp/webapp_server.js
Outdated
WebAppInternals.parsePort = function (port) { | ||
if( /\\\\?.+\\pipe\\?.+/.test(port) ) { | ||
if( /\\\\?.+\\pipe\\?.+/.test(port) || /^(.+)\/([^/]+)$/.test(port) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this new RegExp is much better than just checking to see if the path has a /
in it at all (i.e. /\//
, which is still not ideal. As it stands, this wouldn't allow the socket file to be in the current directory, something we should probably try to support via path.resolve
, for example.
In addition to that, can we also could we consider another option for detecting if it's a TCP socket file? This should also probably check process.platform
(to ensure it's win32
) for the first part.
packages/webapp/webapp_server.js
Outdated
var socketPath = localPort; | ||
var listenOptions = { path: socketPath }; | ||
|
||
httpServer.on('error', Meteor.bindEnvironment(function(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of all this additional logic suggests that these should probably be come more modular functions instead of this rather-large else
block.
packages/webapp/webapp_server.js
Outdated
var listenOptions = { path: socketPath }; | ||
|
||
httpServer.on('error', Meteor.bindEnvironment(function(e) { | ||
if (e.code == 'EADDRINUSE') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this EADDRINUSE
➡️ ECONNREFUSED
logic based on? I'm hesitant to have all this extra logic in here which would normally produce valid error messages anyhow. Can we really safely assume this is stale?
packages/webapp/webapp_server.js
Outdated
if (e.code == 'EADDRINUSE') { | ||
var clientSocket = new net.Socket(); | ||
clientSocket.on('error', Meteor.bindEnvironment(function(e) { | ||
if (e.code == 'ECONNREFUSED') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does a new socket'sECONNREFUSED
error make it safe to cleanup the previous socket file?
packages/webapp/webapp_server.js
Outdated
if (e.code == 'ECONNREFUSED') { | ||
console.log("Deleting stale socket file"); | ||
fs.unlinkSync(socketPath); | ||
startHttpServer(listenOptions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we completely okay with the startHttpServer
getting called again here as it will have already been called on line 859 below?
@abernix In commit d9e7f86 I have rewritten the code to address the majority of your review comments.
If that file is a socket file, it will then try to perform a dummy connection to it to determine if it's live - i.e. belonging to another running process which is listening for connections on it. If the dummy connection attempt is successful, we know the socket file is live and being used by a running process, so we will not delete the file. Instead, we will log an error and exit. If the connection attempt results in the error ECONNREFUSED, we know that the socket file is stale and we can safely delete it using fs.unlinkSync() and call startHttpServer() which will create a fresh socket file.
I really think that old behaviour was wrong and served only to conceal mistakes made by those writing launcher scripts. |
@vlasky Sorry I didn't get to review this today but it's first on my plate tomorrow! (And rest assured, Meteor 1.5.1 is not coming out tomorrow. 😉) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping in mind that this is a major entry point into every application, and in consideration of my comments below, I think this still needs work before it's ready to merge.
Keep in mind that .listen
creates the socket if it doesn't exist, and it also fails if the file does exist already with an error that can be captured. Also, calling .close
on a socket does remove it if the sock file was created.
Aside from the deep nesting (of if
> if
> if
> if
> if
), which could be broken out into more maintainable functions, I think there is still a safer approach here of just trying to startHttpServer
and reacting to the error appropriately, possibly by fixing or checking something and calling back to startHttpServer
again (with a couple retries max).
Thanks for taking a look at this. It won't be ready for 1.5.1, unfortunately. I do still think it's a very valuable change if you're willing to work through tidying it up.
@@ -439,15 +440,6 @@ var getUrlPrefixForArch = function (arch) { | |||
'' : '/' + '__' + arch.replace(/^web\./, ''); | |||
}; | |||
|
|||
// parse port to see if its a Windows Server style named pipe. If so, return as-is (String), otherwise return as Int | |||
WebAppInternals.parsePort = function (port) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize it's called WebAppInternals
, and I think the impact is very low given the general uselessness of this parsePort
function, but this is currently a public API and we should consider the repercussions of removing it or if it's worth keeping in place. (I suspect it was originally only to facilitate testing, but it could have been adopted.)
packages/webapp/webapp_tests.js
Outdated
@@ -156,23 +156,6 @@ Tinytest.add("webapp - generating boilerplate should not change runtime config", | |||
test.isFalse(__meteor_runtime_config__.WEBAPP_TEST_KEY); | |||
}); | |||
|
|||
// Support 'named pipes' (strings) as ports for support of Windows Server / Azure deployments | |||
Tinytest.add("webapp - port should be parsed as int unless it is a named pipe", function(test){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I realize this isn't an exhaustive test by itself, it was testing something and I'm not comfortable removing the only test for this, changing this very core logic and not adding new tests (probably in tools/tests/
). Preferably a few, especially since file-system operations and sockets can behave differently on different operating systems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abernix, in my previous commit 693ac8a, I preserved the legacy parsePort() behaviour and tests, but you weren't happy because socket path would require an absolute path starting with a forward slash, i.e. you could not create a socket file in the current directory.
I note that I have seen other frameworks that use the presence of a forward slash to distinguish between listening on a UNIX socket and listening on a TCP port.
In order to satisfy your concern, I changed this behaviour in commit d9e7f86 so that a number would be treated as a TCP port and anything else would be treated as a UNIX socket, and this required removing the existing parsePort() functionality.
packages/webapp/webapp_server.js
Outdated
if ( /\\\\?.+\\pipe\\?.+/.test(socketPath)) { | ||
startHttpServer(listenOptions); | ||
} else { | ||
if (fs.existsSync(socketPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it still works, fs.existsSync
is technically deprecated (even in Node.js 4) so we should avoid introducing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, running fs.statSync
on the next line is running the same check again but with a race-condition in between. Reacting to the error from statSync
would be better than having two separate calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abernix existsSync
is not deprecated, exists
is however:
https://nodejs.org/api/fs.html#fs_fs_existssync_path
Afaik, exists
was deprecated because it didn't follow the (err, result)
callback signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, bizarre, but true! But still not necessary to use it here. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, existsSync() is not deprecated, however I take @abernix's point about replacing the separate calls to existsSync() and statSync() with a single call to statSync() to avoid a race condition.
packages/webapp/webapp_server.js
Outdated
startHttpServer(listenOptions); | ||
} | ||
})); | ||
clientSocket.connect({ path: socketPath }, function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's typically discouraged to use the net.Socket.prototype.connect
method directly, but rather to rely on net.createConnection
. Something to consider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
net.createConnection() is not suitable here because it is necessary to set the error handler in advance for this very short-lived client socket object whose sole purpose is to distinguish between a live UNIX socket and a stale UNIX socket file.
The documentation does not discourage its use, but says "use this only when implementing a custom socket" or in my view "use it when you have to".
packages/webapp/webapp_server.js
Outdated
startHttpServer(localPort); | ||
} else { | ||
console.error("Invalid PORT specified"); | ||
process.exit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be calling process.exit
anywhere in this code. That will be handled by the caller to main()
here in boot.js
. I think you can just return ERROR
or something (though you should leave the console.error
.
packages/webapp/webapp_server.js
Outdated
process.on('exit', Meteor.bindEnvironment(function(e) { | ||
fs.unlinkSync(socketPath); | ||
})); | ||
process.on('SIGINT', Meteor.bindEnvironment(function(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem relevant to this change at all, what's the reason for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this cleanup code, a stale UNIX socket file (.sock) file is left on disk if you terminate the node process with Control-C (SIGINT).
Hi @vlasky - have you had a chance to look over the recent review comments? |
Hi @vlasky - are you still interested in working on this PR? We'd love to get these changes in place, after the recent review questions have been addressed. If you're not available to work on this let me know, and I'll jump in to help out. Thanks! |
@hwillson I have been away dealing with other business commitments recently. My company has been using commit d9e7f86 in production without any issues. The only change I plan to make to this PR for time being is the one mentioned in #8702 (comment) to resolve a race condition. After that you (or anyone else) can jump in if you see a need. |
No problem at all @vlasky - thanks again for putting this together. After you add in changes to help with the race condition, I'll jump in to help to get this wrapped up / merged. |
…l for race condition when checking for existing UNIX socket file
Just wondering if this is moving forward in 1.5.2 or 1.6 releases as currently tagged as 1.5.1. |
Good question @skirunman - I'm holding this one up unfortunately; hopefully I'll have the discussed changes wrapped up this week. We'll then be able to see which release it makes sense to add this to. Thanks! |
Hi all - I have a new version of this just about ready to go, but I noticed a rather large issue while testing. First off, regarding Let's assume we're using Unfortunately, there isn't a perfectly reliable way to tell if a socket file is already being used by another process. That means we need to handle this check in another way. A few options:
As you can see, neither approach is perfect. I'm leaning towards the first approach and making sure people know that they need to define their socket paths very carefully, to make sure they aren't clobbering another process. This approach falls more in-line with other areas of Meteor, that make certain assumptions to keep the development/deployment process more automatic. I'd love to hear what others think, and if anyone has other suggestions? Thanks! |
Just to add - the existing code in this PR is already achieving |
packages/webapp/webapp_server.js
Outdated
console.error("Error listening:", e); | ||
console.error(e && e.stack); | ||
})); | ||
if (typeof localPort == "string") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
===
?
Thanks @zimme - yes, I have seen that approach and have tried several variations. None of them are perfect unfortunately. If I SIGKILL the running node instance, the sock file is left in place, and I can still connect to it on retry. That means we never actually get a |
I guess using ref/unref won't help either when SIGKILL is used? |
Hi all - based on my comments in #8702 (comment), I've adjusted this PR a bit. If an existing socket file is found when the application starts up, it is first removed before a new socket file is created when the HTTP server starts. As mentioned previously, this could be a bit dangerous if another application is configured to use the same socket file, on the same server. As long as people are configuring their applications properly, and making sure to use unique socket paths for different applications, this really shouldn't be an issue. The good news is by making this assumption up-front, we're able to simplify the codebase (and possible error paths). With this new approach, some of the previous (outstanding) review comments are no longer valid. A quick synopsis:
I believe this addresses all outstanding comments. After these new changes have been reviewed, I'll put a Thanks! |
Adding a test app: https://github.com/hwillson/meteor-pr-8702 |
Shouldn't we try and do cleanup anyway? As we're creating the socket file we should be removing it when we do a graceful shutdown, even though we always remove it before starting again. But what if you don't start it again or change the name of the socket, and all of a sudden you have an unnecessary socket file. |
Good question @zimme - I had exit cleanup code in place just before I committed, but decided to take it out in the end to simplify things. My logic was that we're removing it again at startup, and if a socket file using version of the app is never started again, we've ended up leaving one 0 byte file on the filesystem. I also wasn't crazy about the idea of having additional exit handlers defined in the |
Still littering the filesystem, what if someone setup meteor to always generate a new socket file on boot with a script? Then you might end up with a lot of unused socket files. |
Haha - that would be horrible, but valid point. I'm re-visiting the cleanup code now. Thanks! |
Oh Circle - 4 failures when everything passes locally 🙁. Restarting, but the reported failures shouldn't be related to these changes. Also re-moving the WIP label from the title; this should be ready for a final review. I still have to add a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments!
I think the logic chosen here is great and there doesn't seem to be any de-facto standard on it anyhow.
If anything, this new code (and the code that was there before) is more clean now than it was before, easy to iterate on and looks like a clean execution path in this (relatively critical) entry point. This was a relatively popular feature request, so hopefully we'll get helpful feedback quickly from those who use it and can iterate further if necessary.
// DEPRECATED: Direct use of this function is not recommended; it is no | ||
// longer used internally, and will be removed in a future release. | ||
WebAppInternals.parsePort = port => { | ||
let parsedPort = parseInt(port); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we actually print a deprecation warning to the console, once per session? Or just add this to the History.md
? Or do nothing? (It is called Internals
, after all.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does appear to still be used from webapp_tests.js
. Should we export this function with another internal name for that purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only used in webapp_tests.js
to test itself 🙂. Regarding how to deprecate it, I think just mentioning it in the History.md
should be enough (because as you said, Internals
).
packages/webapp/socket_file_tests.js
Outdated
import { | ||
removeExistingSocketFile, | ||
registerSocketFileCleanup, | ||
} from './socket_file'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we explicitly state the .js
extension? At least for consistency with other code?
packages/webapp/webapp_server.js
Outdated
_.each(callbacks, function (x) { x(); }); | ||
var startHttpServer = function(listenOptions) { | ||
httpServer.listen(listenOptions, Meteor.bindEnvironment(function() { | ||
if (process.env.METEOR_PRINT_ON_LISTEN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit-⛏: Braces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing - I didn't really review that part of the code; I'll update that entire function a bit.
packages/webapp/socket_file_tests.js
Outdated
} from './socket_file'; | ||
import { EventEmitter } from 'events'; | ||
|
||
const testSocketFile = '/tmp/socket_file_tests'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that we test on Windows, but this will fail on Windows. Could we use files.mkdtemp
from tools/fs/files.js
to create a directory for this file to live in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha - oops. Good point - changing, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - I can't reference tools/fs/files.js
directly from webapp
; I'll use Node's os.tmpdir
instead.
Well, I was just prepping a |
Are we sure we don't want to add |
We could - I left those changes out since this PR should be good to go as is and meets the requirements of the original issue (#7392). Adding adjustments for Windows seemed to be a bit out of scope, and the current Windows named pipe hack has been in place for a while, so it's being used in production. Wiring up something like |
Hi all. This is largely our fault, not yours, but I wanted to note for informational purposes that this change played a part in a global outage of self-hosted Sandstorm.io servers. It turns out our startup code would often set a PORT variable with multiple ports, comma-separated, like "80,443". In the past, Meteor would use the first port, and other code in our server would parse out the remaining ports and do other things with them. This change implemented more strict parsing of ports, so broke that configuration. Unfortunately, our usual testing configurations did not happen to have multiple ports set, so the problem was not caught until real servers started dying after auto-updating. Again, this is our fault for questionable code (we should never have relied on lenient PORT parsing) and insufficient testing, but I thought having visibility into this kind of issue would be informative. Sandstorm issue and postmortem here: sandstorm-io/sandstorm#2993 |
@abernix et al, here's my enhancement to implement Support for UNIX sockets (#7392) in meteor.
If the PORT environment variable contains a UNIX file/path, this will be treated as the UNIX domain socket file to listen for incoming connections.
To summarise what I've done:
I added an additional regex check to the existing code in WebAppInternals.parsePort() to test if the PORT variable contains a UNIX file/path and if so, return it as a string.
In function main, we now pass an object called listenOptions to httpServer.listen() and this object contains the required options to configure httpServer to either listen on a TCP port or UNIX socket.
There is a sanity check that handles the situation where there is an existing an existing socket file. If there is, it will try to connect to it to confirm that it does not belong to a running server instance. If it does not, it presumes that the socket file is stale and deletes it.
I note that this support only works in production mode and not when using the meteor tool. I would have liked to add support to the meteor tool, however I see that the package node-http-proxy that it relies on does not appear to support forwarding connections to UNIX sockets, so that would have required a lot more work.