test: test for issue #5574 #5841
Conversation
mscdex
added
readline
|
@piranna A couple of things I've observed:
|
|
I meant: is very slow for a test that runs in the parallel folder. |
There's no problem to increase the speed, in fact it's better. I've put it so slow because I've found a problem where the last characters are lost, and in fact the faster you write the most you receive. Seems it could be related to the
It needs of pull-request #5776 to work, sorry for not specify it before. |
|
If you want to submit the test separately from the fix, the test can go in |
|
Thank for the advice @Trott, I've just done it. |
|
Perhaps @orangemocha could consider having it part of his PR? |
| @@ -0,0 +1,73 @@ | |||
| /** | |||
There was a problem hiding this comment.
Can you follow the conventions of the other files in this directory (first line is strict mode, second line is a Refs:).
There was a problem hiding this comment.
Done, I've left the comment with the description of what's trying to check the test (I think it doesn't hurts...).
The PR for #5574 is ready to go so I'd rather not halt it, but I'll be happy to merge this one once it passes review. Thanks! |
|
@orangemocha cool, thanks for your feedback! |
|
@piranna yes, I agree. Move it back. |
Done. |
Test that characters from stdin are not randomly lost when using a `readline.Interface` and a `tty.ReadStream` on `process.stdin`
|
I've done a rebase over master since #5776 has been merged but now the test on this pull-request fails: === release test-readline-interface-tty-readstream ===
Path: parallel/test-readline-interface-tty-readstream
Assertion failed: (loop->watchers[w->fd] == w), function uv__io_stop, file ../deps/uv/src/unix/core.c, line 864.
assert.js:89
throw new assert.AssertionError({
^
AssertionError: 'SIGABRT' === null
at ChildProcess.<anonymous> (/Users/piranna/github/node/test/parallel/test-readline-interface-tty-readstream.js:57:5)
at ChildProcess.<anonymous> (/Users/piranna/github/node/test/common.js:385:15)
at emitTwo (events.js:101:13)
at ChildProcess.emit (events.js:186:7)
at maybeClose (internal/child_process.js:850:16)
at Socket.<anonymous> (internal/child_process.js:323:11)
at emitOne (events.js:91:13)
at Socket.emit (events.js:183:7)
at Pipe._onclose (net.js:477:12)
Command: out/Release/node /Users/piranna/github/node/test/parallel/test-readline-interface-tty-readstream.jsAny clue what could be happening? Maybe some other commit inserted a regression here? Maybe the problem is related with the |
|
Ok, definitely the test is working, but the |
estliberitas
force-pushed
the
master
branch
2 times, most recently
from
7da4fd4
to
c7066fb
Compare
rvagg
force-pushed
the
master
branch
2 times, most recently
from
c133999
to
83c7a88
Compare
|
maybe @nodejs/streams? See @orangemocha's comment above. |
|
@Fishrock123 I have not much time to perform a git bisect on this, but if you can point me to some instructions to reproduce the problem, I can look at the issue. Also, knowing which commit fixes the issue would make detecting things more clearly. Also, what's the status of this one? |
|
@mcollina This stalled. I think the included test case should surface the problem, or does it not do that anymore? |
|
Should I do anything to run that test? It has been there for minutes, and it is not completing. |
Maybe it got stalled waiting to finish, a timeout would be a good idea. I'm not sure if this problem is still happening, but I suspect it's still the case because there has not be improvements in this area.. |
|
On Mac OS X, I just get a stalled process, I can't reproduce the crash. It is not quitting nor erroring on master or node v6.9.2: |
|
I have just found by serendipity when looking for another bug that this one still happens on Linux with Node.js 6.9.2. |
|
ping... is this still necessary / wanted? |
Yes, it is. In fact, I was checking it just some days ago and the the problem is still there. |
|
Okay, so this has been waiting for way too long, let's see if we can get this landed. @piranna I get the same results as @mcollina , the test just hangs. This PR needs to be rebased on master, and as of #11953, So @piranna do you know why the test is timing out? You can run the test with: tools/test.py --timeout 5 test/parallel/test-readline-interface-tty-readstream.jsto avoid it hanging forever. Diff I applied to run the testdiff --git a/test/parallel/test-readline-interface-tty-readstream.js b/test/parallel/test-readline-interface-tty-readstream.js
index 5d0bce8163..1e6652468d 100644
--- a/test/parallel/test-readline-interface-tty-readstream.js
+++ b/test/parallel/test-readline-interface-tty-readstream.js
@@ -1,4 +1,6 @@
'use strict';
+const common = require('../common');
+
// Refs: https://github.com/nodejs/node/issues/5574
/**
@@ -6,13 +8,10 @@
* `readline.Interface` and a `tty.ReadStream` on `process.stdin`
*/
-var Interface = require('readline').Interface;
-var Readable = require('stream').Readable;
-var ReadStream = require('tty').ReadStream;
-var spawn = require('child_process').spawn;
-var strictEqual = require('assert').strictEqual;
-
-var common = require('../common');
+const Interface = require('readline').Interface;
+const Readable = require('stream').Readable;
+const ReadStream = require('tty').ReadStream;
+const spawn = require('child_process').spawn;
const kpm = 60;
@@ -54,10 +53,10 @@ function grandparent() {
type();
child.on('close', common.mustCall(function(code, signal) {
- strictEqual(signal, null);
- strictEqual(code, 0);
+ assert.strictEqual(signal, null);
+ assert.strictEqual(code, 0);
// cat on windows adds a \r\n at the end.
- strictEqual(output.trim(), input.trim());
+ assert.strictEqual(output.trim(), input.trim());
}));
}
@@ -69,5 +68,5 @@ function parent() {
var stdio = [stdin, process.stdout];
- common.spawnCat({ stdio: stdio });
+ spawn('cat', [], { stdio });
}
|
| /** | ||
| * Test that characters from stdin are not randomly lost when using a | ||
| * `readline.Interface` and a `tty.ReadStream` on `process.stdin` | ||
| */ |
There was a problem hiding this comment.
Nit: Please use the // style for these kinds of comments, just for consistency
| var Readable = require('stream').Readable; | ||
| var ReadStream = require('tty').ReadStream; | ||
| var spawn = require('child_process').spawn; | ||
| var strictEqual = require('assert').strictEqual; |
| var spawn = require('child_process').spawn; | ||
| var strictEqual = require('assert').strictEqual; | ||
|
|
||
| var common = require('../common'); |
There was a problem hiding this comment.
This should be const also, and should be the first import, immediately following the 'use strict' pragma
| const input = `1234567890 | ||
| qwertyuiop | ||
| asdfghjklñ | ||
| zxcvbnm`; |
There was a problem hiding this comment.
Would prefer not using a multiline template literal.
| var child = spawn(process.execPath, [__filename, 'parent']); | ||
| child.stderr.pipe(process.stderr); | ||
|
|
||
| var stdin = Readable({read: function() {}}); |
There was a problem hiding this comment.
const stdin = Readable({ read() {} });
|
One final ping to @piranna ... |
|
Closing due to a long inactivity. @piranna please a comment if you want to follow up on this and we can reopen this or open a new PR instead. |
|
I'm too much busy due to work lately, and also found a valid workaround for this, so not sure what to do now... Seems to me problem is that there are several file descriptors for the TTY streams, so if you pause one of them, the others are still open and capable of reading instead of getting blocked. |
|
@piranna that might be the case. I did not look deeply into this. Do you want to keep on working on this? |
Pull Request check-list
Please make sure to review and check all of these items:
make -j8 test(UNIX) orvcbuild test nosign(Windows) pass withthis change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
test, readline, tty
Description of change
Test that characters from stdin are not randomly lost when using a
readline.Interfaceand atty.ReadStreamonprocess.stdinby writting a text at a slow rate of 60 characters per minute.When tests are executed without the
-j8flag this test show on stderr the messageAssertion failed: (loop->watchers[w->fd] == w), function uv__io_stop, file ../deps/uv/src/unix/core.c, line 864and exit with aSIGABRTsignal. Also, in that case some of the final characters are not received (the faster you write the most you get), probably both problems are related to that previous message.