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

Assertion failed: (uv__stream_fd(stream) >= 0), function uv_read_start, file ../deps/uv/src/unix/stream.c, line 1477. #987

Closed
sergenikitin opened this issue Feb 27, 2015 · 16 comments

Comments

@sergenikitin
Copy link

It happens when I used https://github.com/mscdex/node-imap on io.js 1.4.1 (on mac os).
On <= 1.3.0 node-imap worked good.

@bnoordhuis
Copy link
Member

Can you report this to mscdex/node-imap first, please? It might be a bug in that module that flew under the radar until now. If not, @mscdex should holler and I'll reopen the issue. Thanks.

@vkurchatkin
Copy link
Contributor

It looks like node-imap is pure js, so it is probably io.js bug

@ghost
Copy link

ghost commented Feb 27, 2015

workaround;
deps/uv/src/unix/stream.c
int uv__stream_open(uv_stream_t* stream, int fd, int flags) {

#if defined(__APPLE__)
   enable = 1;
   if (setsockopt(fd, SOL_SOCKET, SO_OOBINLINE, &enable, sizeof(enable)) &&
+      errno != ENOTSOCK) {
-      errno != ENOTSOCK &&
-      errno != EINVAL) {
     return -errno;
   }
#endif

@vkurchatkin
Copy link
Contributor

@gnostic do you have a way to reproduce this issue? what you do reverts libuv/libuv@19d3d50

@mscdex
Copy link
Contributor

mscdex commented Feb 27, 2015

@bnoordhuis As @vkurchatkin said, imap is pure js, so the error is happening in io.js C++ land.

@vkurchatkin vkurchatkin reopened this Feb 27, 2015
@sergenikitin
Copy link
Author

io.js v1.4.2 has the same problem.

@rvagg
Copy link
Member

rvagg commented Mar 1, 2015

@sergenikitin are you on Windows? if so, see #1005 and perhaps #1008 is relevant?

@sergenikitin
Copy link
Author

@rvagg no, OS X 10.10.2

@rvagg
Copy link
Member

rvagg commented Mar 1, 2015

/cc @indutny this looks relevant to your stream changes

@indutny
Copy link
Member

indutny commented Mar 2, 2015

@gnostic does following patch fix the problem for you?

diff --git a/deps/uv/src/unix/stream.c b/deps/uv/src/unix/stream.c
index 518a2fc..ade6b90 100644
--- a/deps/uv/src/unix/stream.c
+++ b/deps/uv/src/unix/stream.c
@@ -393,10 +393,13 @@ int uv__stream_open(uv_stream_t* stream, int fd, int flags) {

 #if defined(__APPLE__)
   enable = 1;
-  if (setsockopt(fd, SOL_SOCKET, SO_OOBINLINE, &enable, sizeof(enable)) &&
-      errno != ENOTSOCK &&
-      errno != EINVAL) {
-    return -errno;
+  if (setsockopt(fd, SOL_SOCKET, SO_OOBINLINE, &enable, sizeof(enable))) {
+    if (errno != ENOTSOCK &&
+        errno != EINVAL) {
+      return -errno;
+    } else {
+      errno = 0;
+    }
   }
 #endif

@sergenikitin
Copy link
Author

@indutny
I checked this patch, it's not fix the problem. Still have

Assertion failed: (uv__stream_fd(stream) >= 0), function uv_read_start, file ../deps/uv/src/unix/stream.c, line 1480.
Abort trap: 6

@indutny
Copy link
Member

indutny commented Mar 3, 2015

@sergenikitin is there any way to reproduce it?

@indutny
Copy link
Member

indutny commented Mar 3, 2015

Ok, a test case:

var tls = require('tls');
var net = require('net');

var socket = new net.Socket();

var s = tls.connect({
  socket: socket,
  servername: 'google.com'
}, function() {
  console.log('secure');
});

socket.connect(443, 'google.com');

@indutny
Copy link
Member

indutny commented Mar 3, 2015

@bnoordhuis @vkurchatkin @rvagg I have two solutions:

  • assume that net.Socket is a JSStream if it has yet no _handle (i.e. .connect() wasn't called)
  • allow adding parent socket handle to TLSSocket after creating it

The first solution is very easy to implement and should generally get various code like that test case working without any problems, except probably reduced bandwidth.

The second one is a bit complicated and might take some time to implement.

What do you think about doing (1) and then (2)?

@bnoordhuis
Copy link
Member

@indutny Is what you are proposing in (1) spiritually equivalent to:

if (!socket._handle) {
  socket.once('connect', function() {
    // continue with tls.connect()?
  });
  return;
}

?

indutny added a commit to indutny/io.js that referenced this issue Mar 3, 2015
Accept `new net.Socket()` as a `socket` option to `tls.connect()`
without triggering an assertion error in C++.

This is done by wrapping it into a JSStream to ensure that there will be
a handle at the time of wrapping the socket into TLSSocket.

Fix: nodejs#987
@indutny
Copy link
Member

indutny commented Mar 3, 2015

@bnoordhuis like this #1046

indutny added a commit that referenced this issue Mar 3, 2015
Accept `new net.Socket()` as a `socket` option to `tls.connect()`
without triggering an assertion error in C++.

This is done by wrapping it into a JSStream to ensure that there will be
a handle at the time of wrapping the socket into TLSSocket.

Fix: #987
PR-URL: #1046
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
@ghost ghost mentioned this issue Jan 15, 2016
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

No branches or pull requests

7 participants