Improve UNIX implementation of uv_guess_handle. #648

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

bennoleslie commented Dec 9, 2012

uv_guess_handle is currently squelching both sockets and fifos
on to the UV_NAMED_PIPE type. Rather than treating sockets
as UV_NAMED_PIPE, they are treated as UV_TCP.

This is not perfect as socket could in fact be UDP sockets,
not TCP sockets, however there does not appear to be a suitable
API for distinguishing between TCP and UDP sockets, and TCP
is still a better guess than simply named pipe.

Additionally, currently all other file descriptor types are
squelched to the UV_FILE type. Instead, only file descriptors
that are marked as regular files are treated as UV_FILE. All
other types (such as directories, character and block devices)
are now treated as UV_UNKNOWN_HANDLE.

Contributor

bnoordhuis commented Dec 10, 2012

Your change makes the stdio_over_pipes test fail. Also, you have a couple of indent errors.

Contributor

bennoleslie commented Dec 14, 2012

A more sophisticated handle guessing routine, that now treats UNIX domain sockets as NAMED_PIPEs (tests now pass), and can also detect UDP sockets.

The current logic is unfortunately pretty deeply nested, but I can't see any way to make it much simpler. Suggestions welcomed.

Contributor

bnoordhuis commented Dec 15, 2012

There are some style issues that I would like to see addressed but the basic premise looks okay to me.

@bnoordhuis bnoordhuis commented on an outdated diff Dec 15, 2012

src/unix/tty.c
@@ -116,7 +116,6 @@ int uv_tty_get_winsize(uv_tty_t* tty, int* width, int* height) {
return 0;
}
-
@bnoordhuis

bnoordhuis Dec 15, 2012

Contributor

Two newlines between functions.

@bnoordhuis bnoordhuis commented on an outdated diff Dec 15, 2012

src/unix/tty.c
@@ -132,11 +131,46 @@ uv_handle_type uv_guess_handle(uv_file file) {
return UV_UNKNOWN_HANDLE;
}
- if (!S_ISSOCK(s.st_mode) && !S_ISFIFO(s.st_mode)) {
+ if (S_ISSOCK(s.st_mode)) {
+ int sock_type;
+ struct sockaddr sockaddr;
+ socklen_t len = sizeof sock_type;
@bnoordhuis

bnoordhuis Dec 15, 2012

Contributor

Declarations go at the top, split up the declaration and the assignment.

@bnoordhuis bnoordhuis commented on an outdated diff Dec 15, 2012

src/unix/tty.c
+ struct sockaddr sockaddr;
+ socklen_t len = sizeof sock_type;
+
+ if (getsockopt(file, SOL_SOCKET, SO_TYPE, &sock_type, &len)) {
+ return UV_UNKNOWN_HANDLE;
+ }
+
+ len = sizeof sockaddr;
+ if (getsockname(file, &sockaddr, &len)) {
+ return UV_UNKNOWN_HANDLE;
+ }
+
+ if (sock_type == SOCK_STREAM && sockaddr.sa_family == AF_UNIX) {
+ return UV_NAMED_PIPE;
+ }
+
@bnoordhuis

bnoordhuis Dec 15, 2012

Contributor

No newline before an else if block.

@bnoordhuis bnoordhuis commented on an outdated diff Dec 15, 2012

src/unix/tty.c
+ }
+
+ else if (sock_type == SOCK_STREAM && sockaddr.sa_family == AF_INET) {
+ return UV_TCP;
+ }
+
+ else if (sock_type == SOCK_DGRAM && sockaddr.sa_family == AF_INET) {
+ return UV_UDP;
+ }
+
+ else {
+ return UV_UNKNOWN_HANDLE;
+ }
+ }
+
+ if (S_ISFIFO(s.st_mode)) {
@bnoordhuis

bnoordhuis Dec 15, 2012

Contributor

If you swap this around - i.e. S_ISFIFO, S_ISREG then if (!S_ISSOCK) return; - you save a level of indenting.

@bnoordhuis bnoordhuis commented on an outdated diff Dec 15, 2012

src/unix/tty.c
+ socklen_t len = sizeof sock_type;
+
+ if (getsockopt(file, SOL_SOCKET, SO_TYPE, &sock_type, &len)) {
+ return UV_UNKNOWN_HANDLE;
+ }
+
+ len = sizeof sockaddr;
+ if (getsockname(file, &sockaddr, &len)) {
+ return UV_UNKNOWN_HANDLE;
+ }
+
+ if (sock_type == SOCK_STREAM && sockaddr.sa_family == AF_UNIX) {
+ return UV_NAMED_PIPE;
+ }
+
+ else if (sock_type == SOCK_STREAM && sockaddr.sa_family == AF_INET) {
@bnoordhuis

bnoordhuis Dec 15, 2012

Contributor

AF_INET || AF_INET6, same for SOCK_DGRAM.

Contributor

bnoordhuis commented Dec 15, 2012

Can you add a couple of tests that check the various handle types? There are a few checks in test/test-tty.c but it's probably best to add a new file test/test-guess-handle.c.

Contributor

bennoleslie commented Jan 1, 2013

Would appreciate some input and suggestions on testing, especially regarding testing on WIN32.

Contributor

txdv commented Jan 1, 2013

Rebase instead of merging in.
uv_sock_t is 64bit on windows, while being a simple 32bit int on LP64 unices.
I don't think the signature is right for this one.

bennoleslie added some commits Dec 9, 2012

@bennoleslie bennoleslie Improve UNIX implementation of uv_guess_handle.
uv_guess_handle is currently squelching both fifo and all
sockets on to the UV_NAMED_PIPE type. Rather than treating
all sockets as UV_NAMED_PIPE, use getsockopt() and
getsockaddr() to determine if the socket is an AF_UNIX
stream (in which case return UV_NAMED_PIPE), or an AF_INET
stream (in which case return UV_TCP), or an AF_INET datagram
socket (in which case return UV_UDP).

Additionally, currently all other file descriptor types are
squelched to the UV_FILE type. Instead, only file descriptors
that are marked as regular files are treated as UV_FILE. All
other types (such as directories, character and block devices)
are now treated as UV_UNKNOWN_HANDLE.
dfb6b35
@bennoleslie bennoleslie Add tests for uv_guess_handle.
This adds relatively comprehensive tests for UNIX platforms,
however the Windows testing is currently light-on. Someone
with strong windows experience may be able to improve this.

The current #ifdef conditional compilation approach is probably
not ideal. It may be simpler to have a seperate test function
for windows and UNIX than the current approach.
47a4780
Contributor

bennoleslie commented Jan 1, 2013

Rebased.
Unclear on the relevance of uv_sock_t or the specific signature being referred to; the implementation is UNIX specific, and the WIN32 tests are a simple lifting of existing tests in test-tty.c.
More specific info appreciated.

Contributor

txdv commented Jan 1, 2013

uv_os_sock is SOCKET which is 8 bytes long on 64bit windows while uv_file is still a simple 4byte long int.

Contributor

txdv commented Jan 1, 2013

O ok, you touched just the unix part, my comment is irrelevant then.

Contributor

bnoordhuis commented Jan 8, 2013

Thanks, landed in 56ed572.

bnoordhuis closed this Jan 8, 2013

Contributor

bnoordhuis commented Jan 8, 2013

Sorry, make that 98bcddc.

Contributor

bnoordhuis commented Jan 11, 2013

I had to revert your patch in 9aab5d4, it was making a lot of the tests in the node.js test suite fail.

Contributor

bennoleslie commented Jan 11, 2013

Is there a link to the node.js failures?

On Sat, Jan 12, 2013 at 12:07 AM, Ben Noordhuis notifications@github.comwrote:

I had to revert your patch in 9aab5d49aab5d4,
it was making a lot of the tests in the node.js test suite fail.


Reply to this email directly or view it on GitHubhttps://github.com/joyent/libuv/pull/648#issuecomment-12143444.

Contributor

bnoordhuis commented Jan 11, 2013

As in a public CI? No, but if you re-apply your patch to node.js with git apply --directory=deps/uv/, you can see it for yourself.

Contributor

bnoordhuis commented Jan 12, 2013

Here is the output of a (partial) test run:

$ python tools/test.py simple
=== release test-listen-fd-cluster ===                                         
Path: simple/test-listen-fd-cluster
Cluster listen fd test []
Cluster listen fd test [ 'parent' ]
about to listen in parent
server listening on 12346
master spawned
Cluster listen fd test [ 'master' ]
in master, spawning worker
output from parent = {"master":5097}


Cluster listen fd test [ 'worker' ]
worker, about to create server and listen on fd=3
node: ../../src/tty_wrap.cc:111: static v8::Handle<v8::Value> node::TTYWrap::GuessHandleType(const v8::Arguments&): Assertion `0' failed.
master exited null

events.js:69
        throw arguments[1]; // Unhandled 'error' event
                       ^
Error: socket hang up
    at createHangUpError (http.js:1331:15)
    at Socket.socketCloseListener (http.js:1381:23)
    at Socket.EventEmitter.emit (events.js:124:20)
    at net.js:421:10
    at process._tickCallback (node.js:386:13)
    at process._makeCallback (node.js:304:15)
Command: out/Release/node /home/bnoordhuis/src/nodejs/master/test/simple/test-listen-fd-cluster.js
=== release test-listen-fd-detached ===                    
Path: simple/test-listen-fd-detached
server listening on 12346
output from parent = {"pid":5111}



events.js:69
        throw arguments[1]; // Unhandled 'error' event
                       ^
Error: socket hang up
    at createHangUpError (http.js:1331:15)
    at Socket.socketCloseListener (http.js:1381:23)
    at Socket.EventEmitter.emit (events.js:124:20)
    at net.js:421:10
    at process._tickCallback (node.js:386:13)
    at process._makeCallback (node.js:304:15)
Command: out/Release/node /home/bnoordhuis/src/nodejs/master/test/simple/test-listen-fd-detached.js
=== release test-listen-fd-detached-inherit ===                    
Path: simple/test-listen-fd-detached-inherit
server listening on 12346
output from parent = {"pid":5122}


node: ../../src/tty_wrap.cc:111: static v8::Handle<v8::Value> node::TTYWrap::GuessHandleType(const v8::Arguments&): Assertion `0' failed.

events.js:69
        throw arguments[1]; // Unhandled 'error' event
                       ^
Error: socket hang up
    at createHangUpError (http.js:1331:15)
    at Socket.socketCloseListener (http.js:1381:23)
    at Socket.EventEmitter.emit (events.js:124:20)
    at net.js:421:10
    at process._tickCallback (node.js:386:13)
    at process._makeCallback (node.js:304:15)
Command: out/Release/node /home/bnoordhuis/src/nodejs/master/test/simple/test-listen-fd-detached-inherit.js
=== release test-listen-fd-server ===                             
Path: simple/test-listen-fd-server
server listening on 12346
child spawned
output from parent = {"pid":5135}


node: ../../src/tty_wrap.cc:111: static v8::Handle<v8::Value> node::TTYWrap::GuessHandleType(const v8::Arguments&): Assertion `0' failed.
child exited null

events.js:69
        throw arguments[1]; // Unhandled 'error' event
                       ^
Error: socket hang up
    at createHangUpError (http.js:1331:15)
    at Socket.socketCloseListener (http.js:1381:23)
    at Socket.EventEmitter.emit (events.js:124:20)
    at net.js:421:10
    at process._tickCallback (node.js:386:13)
    at process._makeCallback (node.js:304:15)
Command: out/Release/node /home/bnoordhuis/src/nodejs/master/test/simple/test-listen-fd-server.js

It looks like the addition of UV_TCP and UV_UDP is breaking TTYWrap::GuessHandleType().

Contributor

bnoordhuis commented Jan 12, 2013

The patch below fixes all tests except simple/test-listen-fd-detached. I don't have time to look into it but take the patch and run with it if you want.

diff --git a/lib/net.js b/lib/net.js
index 89cb0d7..8199cf0 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -911,6 +911,7 @@ var createServerHandle = exports._createServerHandle =
     var type = tty_wrap.guessHandleType(fd);
     switch (type) {
       case 'PIPE':
+      case 'TCP':
         debug('listen pipe fd=' + fd);
         // create a PipeWrap
         handle = createPipe();
diff --git a/src/node.js b/src/node.js
index d24387c..d0e81bd 100644
--- a/src/node.js
+++ b/src/node.js
@@ -481,6 +481,7 @@
         break;

       case 'PIPE':
+      case 'TCP':
         var net = NativeModule.require('net');
         stream = new net.Socket({
           fd: fd,
diff --git a/src/tty_wrap.cc b/src/tty_wrap.cc
index 1751105..ca5b362 100644
--- a/src/tty_wrap.cc
+++ b/src/tty_wrap.cc
@@ -95,9 +95,15 @@ Handle<Value> TTYWrap::GuessHandleType(const Arguments& args) {
   uv_handle_type t = uv_guess_handle(fd);

   switch (t) {
+    case UV_TCP:
+      return scope.Close(String::New("TCP"));
+
     case UV_TTY:
       return scope.Close(String::New("TTY"));

+    case UV_UDP:
+      return scope.Close(String::New("UDP"));
+
     case UV_NAMED_PIPE:
       return scope.Close(String::New("PIPE"));
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment