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

IPC Unix domain socket / Windows named pipe #3742

Merged
merged 3 commits into from Nov 28, 2017
Merged

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Nov 15, 2017

Fixes #3723

In addition to TCP, this adds support for IPC (interprocess communication) using Unix domain socket and Windows named pipe. The Java socket implementation for IPC is lifted from facebook/nailgun, which is released under Apache v2.

The use of Unix domain socket has performance and security benefits.

Here's screenshot of this running on a Windows virtual machine:
windows_7_64bit__running_

serverConnectionType

This introduces a new setting key called serverConnectionType, which is set to ConnectionType.Local by default:

serverConnectionType := ConnectionType.Local,

The other option is ConnectionType.Tcp.

active.json

To communicate to the clients about the connection type, it uses URI with local: as the protocol.

{"uri":"local:///Users/xxx/.sbt/1.0/server/0845deda85cb41abdb9f/sock"}

@eed3si9n
Copy link
Member Author

Travis passed the test. Codacy problems are coming from the Nailgun code's variable names.

@dwijnand
Copy link
Member

Why does this include a bunch of Nailgun source code?

@eed3si9n
Copy link
Member Author

@dwijnand The only available nailgun artifact on Maven Central is 0.9.1, and the code that I am borrowing from them is fairly new stuff for nailgun 2.x.

Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Please could you:

  1. create an issue requesting this nailgun classes be released somewhere, and
  2. add somewhere a reference to what commit sha these source files come from (for future reference)

prepareSocketfile()
new NGUnixDomainServerSocket(socketfile.getAbsolutePath)
case ConnectionType.Tcp => new ServerSocket(port, 50, InetAddress.getByName(host))
case ConnectionType.Ssh => sys.error("ssh it not yet supported")
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe it's best to introduce this enum value when it's supported.

In addition to TCP, this adds sbt server support for IPC (interprocess communication) using Unix domain socket and Windows named pipe.

The use of Unix domain socket has performance and security benefits.
@eed3si9n
Copy link
Member Author

@dwijnand
Copy link
Member

Is there an issue about having a binary release on Maven Central?

@eed3si9n
Copy link
Member Author

Is there an issue about having a binary release on Maven Central?

Binary release for Nailgun? 0.9.1 from 2012 is published, but there hasn't been public release yet since. The interesting bits that I am after were committed in 2017. It could mean that 0.9.2 is not yet out, or ppl are releasing in-house as app.

@dwijnand
Copy link
Member

Binary release for Nailgun?

Either of nailgun or a subset of nailgun that includes the classes we're importing here.

I see you've created facebookarchive/nailgun#108 (comment) - I'd've preferred a standalone, more easily findable, issue, but I'm not going to make that get in the way of this landing.

@dwijnand dwijnand merged commit 297fd5d into sbt:1.x Nov 28, 2017
@jameskoch2
Copy link

jameskoch2 commented Nov 29, 2017

Would there be any security benefit to applying these same Sockets to ForkTests? Reading the code it seems like there's a very small window between ForkTests opening a loopback port and accepting at-most-one connections from the child process. So benefit seems minimal, at a glance, but would like to hear other opinions.

@eed3si9n
Copy link
Member Author

I think any inter-process communication is worth reviewing to see either Unix domain socket and/or server is applicable.

@jameskoch2
Copy link

I filed #3938 after closer inspection of the NamedPipes code, because by my reading it looks like it would still allow other users on the same machine to snoop on traffic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants