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

iojs 1.4.x, linux: ../deps/uv/src/unix/stream.c:1174: uv_shutdown; win: shutdown EPIPE #1068

Closed
qfox opened this issue Mar 5, 2015 · 25 comments
Assignees
Labels
confirmed-bug Issues with confirmed bugs.

Comments

@qfox
Copy link

qfox commented Mar 5, 2015

$ uname -a
Linux dev.qfox.ru 3.2.0-4-486 #1 Debian 3.2.51-1 i686 GNU/Linux

Expected as it was:

$ iojs -v
v1.3.0
$ iojs -e 'process.stdin.emit("end");'

Silent exit.

Actual:

$ iojs -v
v1.4.3
$ iojs -e 'process.stdin.emit("end");'
iojs: ../deps/uv/src/unix/stream.c:1174: uv_shutdown: Assertion `(stream->type == UV_TCP || stream->type == UV_NAMED_PIPE) && "uv_shutdown (unix) only supports uv_handle_t right now"' failed.
Аварийный останов

cc @indutny

upd Btw, this code also produces strange error on win7 x64 (running in mingw32):

$ iojs -v
v1.4.3
$ iojs -e 'process.stdin.emit("end");'
events.js:141
      throw er; // Unhandled 'error' event
            ^
Error: shutdown EPIPE
    at exports._errnoException (util.js:734:11)
    at ReadStream.onSocketFinish (net.js:218:26)
    at emitNone (events.js:67:13)
    at ReadStream.emit (events.js:163:7)
    at finishMaybe (_stream_writable.js:477:14)
    at endWritable (_stream_writable.js:486:3)
    at ReadStream.Writable.end (_stream_writable.js:452:5)
    at ReadStream.Socket.end (net.js:393:31)
    at process._tickCallback (node.js:350:11)
@qfox qfox changed the title iojs 1.4.x, linux: ../deps/uv/src/unix/stream.c:1174: uv_shutdown iojs 1.4.x, linux: ../deps/uv/src/unix/stream.c:1174: uv_shutdown; win: shutdown EPIPE Mar 5, 2015
@bnoordhuis
Copy link
Member

Confirmed. Do you happen to know if older io.js versions are affected? If not, I'll just run git bisect.

@bnoordhuis bnoordhuis added the confirmed-bug Issues with confirmed bugs. label Mar 5, 2015
@qfox
Copy link
Author

qfox commented Mar 5, 2015

@bnoordhuis Affected versions are 1.4.1, 1.4.2, 1.4.3.
1.3.0 is fine.

@bnoordhuis
Copy link
Member

@zxqfox Thanks. I'll have a look.

@Fishrock123
Copy link
Contributor

Also happens on OS X. (Same error as Linux)

@bnoordhuis
Copy link
Member

b9686233fc0be679d7ba1262b611711629ee334e is the first bad commit
commit b9686233fc0be679d7ba1262b611711629ee334e
Author: Fedor Indutny <fedor@indutny.com>
Date:   Sun Feb 22 21:59:07 2015 +0300

    stream_base: introduce StreamBase

    StreamBase is an improved way to write C++ streams. The class itself is
    for separting `StreamWrap` (with the methods like `.writeAsciiString`,
    `.writeBuffer`, `.writev`, etc) from the `HandleWrap` class, making
    possible to write abstract C++ streams that are not bound to any uv
    socket.

    The following methods are important part of the abstraction (which
    mimics libuv's stream API):

    * Events:
      * `OnAlloc(size_t size, uv_buf_t*)`
      * `OnRead(ssize_t nread, const uv_buf_t*, uv_handle_type pending)`
      * `OnAfterWrite(WriteWrap*)`
    * Wrappers:
      * `DoShutdown(ShutdownWrap*)`
      * `DoTryWrite(uv_buf_t** bufs, size_t* count)`
      * `DoWrite(WriteWrap*, uv_buf_t*, size_t count, uv_stream_t* handle)`
      * `Error()`
      * `ClearError()`

    The implementation should provide all of these methods, thus providing
    the access to the underlying resource (be it uv handle, TLS socket, or
    anything else).

    A C++ stream may consume the input of another stream by replacing the
    event callbacks and proxying the writes. This kind of API is actually
    used now for the TLSWrap implementation, making it possible to wrap TLS
    stream into another TLS stream. Thus legacy API calls are no longer
    required in `_tls_wrap.js`.

    PR-URL: https://github.com/iojs/io.js/pull/840
    Reviewed-By: Trevor Norris <trev.norris@gmail.com>
    Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>

/cc @indutny

@indutny
Copy link
Member

indutny commented Mar 5, 2015

Ok, on my plate then :)

@indutny indutny self-assigned this Mar 5, 2015
indutny added a commit to indutny/io.js that referenced this issue Mar 5, 2015
UV_TTY does not support `uv_shutdown()` so adding this method in
StreamBase will cause an `abort()` in C land.

Fix: nodejs#1068
@indutny
Copy link
Member

indutny commented Mar 5, 2015

Should be fixed by #1073.

indutny added a commit that referenced this issue Mar 5, 2015
UV_TTY does not support `uv_shutdown()` so adding this method in
StreamBase will cause an `abort()` in C land.

Fix: #1068
PR-URL: #1073
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@indutny
Copy link
Member

indutny commented Mar 5, 2015

Fixed!

@indutny indutny closed this as completed Mar 5, 2015
@Fishrock123
Copy link
Contributor

@indutny the test case for this is still failing: win2012r2 & win2008r2

@indutny
Copy link
Member

indutny commented Mar 6, 2015

Gosh, cc @piscisaureus: PTAL

@qfox
Copy link
Author

qfox commented Mar 6, 2015

Just fyi: I can't repeat it on my local win machine with v1.4.4-nightly20150305b27931b0fe

@indutny
Copy link
Member

indutny commented Mar 6, 2015

Neither do I.

@rvagg
Copy link
Member

rvagg commented Mar 19, 2015

@indutny, @piscisaureus: the failure of test-regress-GH-io-1068 is the last big item in #1005 that we need to sort out for Windows. The behaviour exhibits when run from within Jenkins but does not when you run the tests or the command directly with iojs -e ... I'm guessing it's the fact that when run from under Jenkins it's not actually a TTY so kFlagNoShutdown is not set on that stream, but I have no idea what else it would be if not a TTY and why this would be a problem on Windows. Is there another type of system stream that's not happy with the shutdown?

@indutny
Copy link
Member

indutny commented Mar 19, 2015

@rvagg is there any way to check if the test was failing before the streambase stuff? If so - we may want to just disable it.

@Fishrock123
Copy link
Contributor

@indutny looks like @bnoordhuis pointed out above in #1068 (comment) that it originated from stream_base..

@indutny
Copy link
Member

indutny commented Mar 19, 2015

@Fishrock123 yeah, but it the unintentional behavior change was fixed since then. So practically io.js should behave the same way as it was doing before the StreamBase. That's why I'm asking about reproducing it on previous io.js versions.

@rvagg
Copy link
Member

rvagg commented Mar 19, 2015

@indutny you could make a branch in your own repo from just before StreamBase and put in that regress test and run it through CI and that'll tell you ... you could do an awkward bisect using that method too I suppose, if you have patience.

@indutny
Copy link
Member

indutny commented Mar 19, 2015

Here it is, right before the StreamBase: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/348/ (crossing my fingers :) )

@rvagg
Copy link
Member

rvagg commented Mar 19, 2015

IMO even if this does turn out to exist prior to StreamBase it's now something that we have to find a way to deal with, it's just a bug that was previously unexposed. I'd like some thought put in to what conditions are causing this and how to "fix" it so that it doesn't throw.

@indutny
Copy link
Member

indutny commented Mar 19, 2015

@rvagg sure thing. I'll probably need to spend some time reproducing it, though.

@rvagg
Copy link
Member

rvagg commented Mar 19, 2015

I spent some time yesterday trying to repro it with child_process gymnastics to no avail, perhaps we can use python to invoke something similar to what's going on with Jenkins

@indutny
Copy link
Member

indutny commented Mar 19, 2015

And the build is blue, right before the StreamBase. I'll see what I could have broke there.

@indutny
Copy link
Member

indutny commented Mar 21, 2015

Yay, I was able to reproduce it:

echo 1 | ./Release/iojs.exe test/parallel/test-regress-GH-io-1068.js

Will figure out the problem in a bit.

@indutny
Copy link
Member

indutny commented Mar 21, 2015

Good news, test appears to be failing right before the StreamBase changes too. Perhaps this is still unrelated to the stuff that we see on Jenkins.

indutny added a commit to indutny/io.js that referenced this issue Mar 21, 2015
Stdin is purely read-only stream. Although, `net.Socket` might be used
to create it if stdin is in fact a Pipe or TCP socket, the
`stream.Duplex` should not try to call `.end()` on it.

Fix: nodejs#1068
indutny added a commit that referenced this issue Mar 22, 2015
Stdin is purely read-only stream. Although, `net.Socket` might be used
to create it if stdin is in fact a Pipe or TCP socket, the
`stream.Duplex` should not try to call `.end()` on it.

Fix: #1068
PR-URL: #1233
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@indutny
Copy link
Member

indutny commented Mar 22, 2015

@rvagg: windows CI failures should be resolved now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

No branches or pull requests

5 participants