LGTM

I think @rvagg aborted my CI trial run. Here's another: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/201/

Sorry if I did, quite possible. I also just aborted the armv6 part of that cause I need it for release but I doubt that would have been interesting anyway.

And of course VS 2013 chokes on the template function... The day we ditch VS for clang can not come quickly enough! Pushed a fix and started a new CI run, PTAL. /cc @piscisaureus

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/203/

@rvagg could we re-run the tests on the windows machines (assuming we know they now give us proper test feedback)?

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/223/

+1, I think this is a big improvement in the API, but:

• I think it necessitates a major bump (I can search around, but I'm pretty sure I have code calling address() that will break).
• The PR includes no changes to documentation

For those wanting to see the before vs after, with

var net = require('net');

var c;
var s = net.createServer().listen('_pipe').on('listening', function() {
  console.log('server address:', this.address());

  c = net.connect('_pipe').on('connect', function() {
    console.log('client address:', this.address());
  });
});

Before this PR:

% rm -f _pipe; node pn.js  
server address: _pipe
client address: {}

After:

% rm -f _pipe; ./iojs pn.js                         
server address: { address: '_pipe' }
client address: { address: '' }

Much better, but not API compatible.

@bnoordhuis and that client address: { address: '' }... that's a bug, isn't it?

@sam-github Nope, that's just how UNIX sockets work. The client doesn't have a local address.

@bnoordhuis at the system level, yes, but we could save the address we connected to and return it. Using an address object rather than a string already makes this more consistent with TCP, papering over this last thing would make it even better.

@sam-github That's what socket.remoteAddress is for. The client socket itself isn't addressable, that's why it's an empty string. I could set the address to null or undefined but that just forces people to write more guards in their code.

@bnoordhuis TCP client sockets aren't addressable, either, but

client address: { address: '127.0.0.1', family: 'IPv4', port: 40638 }

I take your point that the client port is at least unique to the client, unlike in the unix domain.

But as it is, the check to see if a client is unix or TCP now is:

if (sock.address().address === '')
  // unix
else
  // tcp

Maybe at least the address family could be present.

@sam-github Added .family property and documentation, PTAL.

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/243/

There are 11 failures on Windows but only 2 are related to this PR, parallel/test-http-unix-socket and sequential/test-pipe-address. Apparently the dot in the file path gets rewritten to a question mark, not sure what's up with that.

@piscisaureus Is that expected behavior? From looking at libuv, the named pipe's name seems to be coming directly from NtQueryInformationFile().

@bnoordhuis
You mean that the path prefix changes from `.\pipe\something' into '?\pipe\something' ?
That's expected, and not something we can really fix. If you want the long answer ask for it :)

Morbid fascination gets the better of me: what's the long answer? Should I just special-case it in tests or can I change common.PIPE to \\?\\pipe\\something? I'm guessing the answer is 'no.'

@bnoordhuis
The long answer is that the the Win32 layer processes DOS paths and paths that start with \\.\ into a form that starts with \\?\. As part of the transformation . and .. components are squelched, forward slashes become backward slashes, trailing whitespace and dots are stripped from all path components.
The NT kernel doesn't store the original path, only the \\?\ result.

A way to make the test pass is to use a \\?\pipe\ path to begin with, it'll come out unmodified.

Alright, thanks for the answer. I'll update test/common.js.

@bnoordhuis
To appease your morbid fascination a bit further, this is also the reason that symlink resolution differs between windows and unix. On windows the path is essentially "resolved" by the Win32 layer, as the NT layer does not support .. as a reference to the above directory.

The pipe filesystem driver however is a bit different, since it does not support directories. Any pipe name is acceptable, even slashes are considered part of the file name. However due to the '..' processing in the Win32 layer, one can use the pipe filesystem as if it supported directories. That is, with the exception that there's no possibility nor need to create directories.

@piscisaureus Duly noted, thanks. :-) Here's another try: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/249/

Feature as implemented LGTM

Though two things to consider:

In the server.listen(path, ... docs, I took the approach of describing these sockets as "local", defining what local is on Unix and Windows, and then just using "local sockets" throughout the documentation, rather than repeating "Unix sockets and Windows pipes" everywhere.

I think its a semver-major API change. Not a big API change, not world changing, not going to effect 99% of node code (because its all HTTP), not worth reving to 2.x tomorrow because of it... but still an API change.

I worry that not calling it major means iojs isn't semvered again... its going back to "major is a BIG change", rather than "major means some of your code that previously conformed to the documented API may no longer work as it used to".

Boo! Looks like uv_pipe_getpeername() doesn't work quite the same way on Windows as it does on UNIX.

=== release test-http-unix-socket ===
Path: parallel/test-http-unix-socket
assert.js:87
  throw new assert.AssertionError({
        ^
AssertionError: '' == '\\\\?\\pipe\\libuv-test'
    at Server.<anonymous> (c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\test\parallel\test-http-unix-socket.js:11:10)
    at emitTwo (events.js:87:13)
    at Server.emit (events.js:169:7)
    at HTTPParser.parserOnIncoming [as onIncoming] (_http_server.js:471:12)
    at HTTPParser.parserOnHeadersComplete (_http_common.js:88:23)
    at Socket.socketOnData (_http_server.js:322:22)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:166:7)
    at readableAddChunk (_stream_readable.js:146:16)
    at Socket.Readable.push (_stream_readable.js:109:10)
Command: c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\Release\iojs.exe c:\workspace\iojs+any-pr+multi\nodes\iojs-win2012r2\test\parallel\test-http-unix-socket.js

In the server.listen(path, ... docs, I took the approach of describing these sockets as "local", defining what local is on Unix and Windows, and then just using "local sockets" throughout the documentation, rather than repeating "Unix sockets and Windows pipes" everywhere.

@sam-github That seems less than optimal. The reference documentation is not a document that people normally read top to bottom, they just skip to the section they're interested in. If you explain something in the middle, they're going to miss it.

I think its a semver-major API change. Not a big API change, not world changing, not going to effect 99% of node code (because its all HTTP), not worth reving to 2.x tomorrow because of it... but still an API change.

Virtual shrug. Personally, I think it's closer to a bug fix than a major change because of this (from here):

socket.address() returns an empty object if it's not connected over AFINET.

Of course it's not an empty object but the path string getting coerced to an object in code like socket.address().address.

@iojs/tc What do you think?

CI looks good.

Landed in d6714ff...7a999a1. GH got a little confused because the target branch changed from v1.x to master.

@Fishrock123 @bnoordhuis Looks like the common.PIPE changes in this PR breaks the CI. https://jenkins-iojs.nodesource.com/job/node-test-commit-windows/345/nodes=win2012r2/tapTestReport/test.tap-403/ and https://jenkins-iojs.nodesource.com/job/node-test-commit-windows/345/nodes=win2008r2/tapTestReport/test.tap-403/

Curious. I'm unclear on why the previous run succeeded. I'm doing a new run to see if it's reproducible on master: https://jenkins-iojs.nodesource.com/job/node-test-commit/359/

@bnoordhuis That particular case failed again in both the Windows instances.

I think I understand now why the previous CI run passed, it was building the wrong PR. There was a hiccup a few days ago where some runs got relabeled because of another, aborted run and I suspect I've hit that. I'll see about fixing the tests.

Cool then :)

Testing a hunch here. It looks like uv_pipe_getsockname() on Windows is basically a no-op for client sockets...

@bnoordhuis No luck :-( That test is still failing.

Moving to revert, see #2584.