FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix test-readline-interface by Trott · Pull Request #14681 · nodejs/node · GitHub

/ node Public

fix test-readline-interface - #14681

Closed
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:fix-readline
ConversationCommits (2)ChecksFiles changed
Closed

fix test-readline-interface#14681
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:fix-readline

Conversation

Trott commented Aug 8, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

First commit is from #14677 and I will remove it and rebase if that PR lands before this one (which I imagine it will).

Second First commit (h/t @addaleax):

lib: use Timer.now() in readline module

Using Date.now() introduces problems when operating under load or
otherwise with constrained resources. Use Timer.now() to mitigate.

Refs: https://github.com/nodejs/node/issues/14674

Third Second commit:

test: split out load-sensitive readline tests

Two test cases in `test-readline-interface` are sensitive to resource
constraints (probably due to `\r` and `\n` not arriving within the
appropriate delay to be treated as a single line ending). Move those
tests to `sequential`.

Fixes: https://github.com/nodejs/node/issues/14674
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test readline

nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Aug 8, 2017

Trott commented Aug 8, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/9531/

Trott mentioned this pull request Aug 8, 2017
Closed
cjihrig approved these changes Aug 8, 2017
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM pending CI

Trott commented Aug 8, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

arm-fanned is green! Ci is effectively green. (Only failure is that vexing C++ test failure on armv7-wheezy.)

refack suggested changes Aug 8, 2017
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I starting to RC Collaborators, since our review process is getting watered down.

Comment thread test/sequential/test-readline-interface.js Outdated

refack Aug 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please comment what this tests, and why is it in /sequential/.

Trott Aug 8, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Comment added.

Trott Aug 8, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality 
// These test cases are in `sequential` rather than the analogous test file in
// `parallel` because they become unrelaible under load. The unreliability under
// load was determined empirically when the test cases were in `parallel` by
// running:
//   tools/test.py -j 96 --repeat 192 test/parallel/test-readline-interface.js

refack Aug 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

AFAIK it should be in the code, not just in GitHub 🤔

Trott Aug 8, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's puzzling. I must have force-pushed it out at some point...

Trott Aug 8, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Argh! I overwrote the commit message change I did for @bnoordhuis too. The joys of having two laptops and forgetting to fetch before starting work, I imagine...

Trott Aug 8, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

OK, all fixes have been re-fixed.

addaleax commented Aug 8, 2017

Copy link
Copy Markdown
Member

@Trott is moving to sequential actually necessary after the first adjustments? I think we might not need that, libuv’s now() should work well with synchronous input

bnoordhuis commented Aug 8, 2017

Copy link
Copy Markdown
Member

Using Date.now() introduces problems when operating under load or otherwise with constrained resources.

Could use more detail. What problems, what load or constrained resources?

Trott commented Aug 8, 2017

Copy link
Copy Markdown
Member Author

@Trott is moving to sequential actually necessary after the first adjustments? I think we might not need that, libuv’s now() should work well with synchronous input

@addaleax Without the third commit, here's the sorts of failures those two test cases get (that were moved to sequential) when resource-constrained:

Details 
$ tools/test.py -j 96 --repeat 192 test/parallel/test-readline-interface.js 
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bat'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:262:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bat'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bat'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'baz'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'baz'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'baz'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1032
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:287:10)
    at Interface._line (readline.js:642:8)
    at Interface._ttyWrite (readline.js:929:16)
    at FakeInput.onkeypress (readline.js:165:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
[00:09|% 100|+ 160|-  32]: Done  
$

The changes to lib/readline.js solved a bunch of other issues in the test, but I think the above problems are an artifact of the way those two test cases are written. High load makes them unreliable because subsequent .write() calls do not happen quickly enough. At least, that's what seems to be happening to me.

Trott commented Aug 8, 2017

Copy link
Copy Markdown
Member Author

Could use more detail. What problems, what load or constrained resources?

@bnoordhuis The problem was diagnosed (and the solution prposed) by @addaleax in #14674 (comment):

the delay between processing \r and \n is so long that readline thinks they are actually separate line endings.

I do think it’s problematic that this happens even for synchronously emitted input. Maybe we should be using process.binding('timer_wrap').Timer.now() instead of Date.now()?

As for load, I tend to simulate by running many test processes simultaneously which seems to pretty reliably predict (or retroactively "predict") what tests are unreliable in CI:

$ tools/test.py -j 96 --repeat 192 test/parallel/test-foo-bar.js

bnoordhuis commented Aug 8, 2017

Copy link
Copy Markdown
Member

Right, I mean the commit log could go into more detail. I kind of gleaned what you wrote from the linked issue but it's better for the commit log to document that.

Trott commented Aug 8, 2017

Copy link
Copy Markdown
Member Author

@bnoordhuis Updated commit message.

lib: use Timer.now() in readline module

Using Date.now() introduces problems when operating under load or
otherwise with constrained resources. Use Timer.now() to mitigate.

The problem was identified in `test-readline-interface` where under
heavy load, `\r` and `\n` were received so far apart that they were
treated as separate line endings rather than a single line ending.
Switching to `Timer.now()` prevented this from happening.

Refs: https://github.com/nodejs/node/issues/14674

Trott force-pushed the fix-readline branch 3 times, most recently from c4a660e to 3fd215c Compare August 8, 2017 14:26
MylesBorins mentioned this pull request Aug 8, 2017
Merged

refack commented Aug 8, 2017

Copy link
Copy Markdown
Contributor

@Trott after f11379d is it still falky (I assume yes, but do we have empirical evidance)?

Trott added 2 commits August 8, 2017 12:58
lib: use Timer.now() in readline module
9f1370f 
Using Date.now() introduces problems when operating under load or
otherwise with constrained resources. Use Timer.now() to mitigate.

The problem was identified in `test-readline-interface` where under
heavy load, `\r` and `\n` were received so far apart that they were
treated as separate line endings rather than a single line ending.
Switching to `Timer.now()` prevented this from happening.

Refs: nodejs#14674
test: split out load-sensitive readline tests
710304c 
Two test cases in `test-readline-interface` are sensitive to resource
constraints (probably due to `\r` and `\n` not arriving within the
appropriate delay to be treated as a single line ending). Move those
tests to `sequential`.

Fixes: nodejs#14674

Trott commented Aug 8, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@Trott after f11379d is it still falky (I assume yes, but do we have empirical evidance)?

@refack f11379d seems to fix the flakiness on Raspberry Pi 1 in CI, but there is still unreliability when the test is run under load, which is what this PR addresses.

Details 
$ tools/test.py -j 96 --repeat 192 test/parallel/test-readline-interface.js 
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:262:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 2 === 1
    at Timeout.common.mustCall (/Users/trott/io.js/test/parallel/test-readline-interface.js:349:14)
    at Timeout._onTimeout (/Users/trott/io.js/test/common/index.js:509:15)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                    
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:262:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:262:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
=== release test-readline-interface ===                   
Path: parallel/test-readline-interface
readline.js:1030
            throw err;
            ^

AssertionError [ERR_ASSERTION]: '' === 'bar'
    at Interface.<anonymous> (/Users/trott/io.js/test/parallel/test-readline-interface.js:245:14)
    at emitOne (events.js:115:13)
    at Interface.emit (events.js:210:7)
    at Interface._onLine (readline.js:285:10)
    at Interface._line (readline.js:640:8)
    at Interface._ttyWrite (readline.js:927:16)
    at FakeInput.onkeypress (readline.js:163:10)
    at emitTwo (events.js:125:13)
    at FakeInput.emit (events.js:213:7)
    at emitKeys (internal/readline.js:420:14)
Command: out/Release/node --expose_internals /Users/trott/io.js/test/parallel/test-readline-interface.js
[00:08|% 100|+ 176|-  16]: Done  
$

(With the changes in this PR, the test passes with the above command line.)

Trott commented Aug 10, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/9582/

Trott commented Aug 10, 2017

Copy link
Copy Markdown
Member Author

AIX failure is almost certainly unrelated, but just in case...

CI: https://ci.nodejs.org/job/node-test-pull-request/9585/

Trott added a commit to Trott/io.js that referenced this pull request Aug 10, 2017
lib: use Timer.now() in readline module
fe54bc7 
Using Date.now() introduces problems when operating under load or
otherwise with constrained resources. Use Timer.now() to mitigate.

The problem was identified in `test-readline-interface` where under
heavy load, `\r` and `\n` were received so far apart that they were
treated as separate line endings rather than a single line ending.
Switching to `Timer.now()` prevented this from happening.

PR-URL: nodejs#14681
Refs: nodejs#14674
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Trott added a commit to Trott/io.js that referenced this pull request Aug 10, 2017
test: split out load-sensitive readline tests
c7a9a2e 
Two test cases in `test-readline-interface` are sensitive to resource
constraints (probably due to `\r` and `\n` not arriving within the
appropriate delay to be treated as a single line ending). Move those
tests to `sequential`.

PR-URL: nodejs#14681
Fixes: nodejs#14674
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>

Trott commented Aug 10, 2017

Copy link
Copy Markdown
Member Author

Landed in fe54bc7 and c7a9a2e

Trott closed this Aug 10, 2017
addaleax pushed a commit that referenced this pull request Aug 10, 2017
lib: use Timer.now() in readline module
8433b1a 
Using Date.now() introduces problems when operating under load or
otherwise with constrained resources. Use Timer.now() to mitigate.

The problem was identified in `test-readline-interface` where under
heavy load, `\r` and `\n` were received so far apart that they were
treated as separate line endings rather than a single line ending.
Switching to `Timer.now()` prevented this from happening.

PR-URL: #14681
Refs: #14674
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax mentioned this pull request Aug 10, 2017
Closed
2 tasks
addaleax pushed a commit that referenced this pull request Aug 10, 2017
test: split out load-sensitive readline tests
3f1bb0a 
Two test cases in `test-readline-interface` are sensitive to resource
constraints (probably due to `\r` and `\n` not arriving within the
appropriate delay to be treated as a single line ending). Move those
tests to `sequential`.

PR-URL: #14681
Fixes: #14674
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
addaleax mentioned this pull request Aug 13, 2017
Merged

MylesBorins commented Sep 19, 2017

Copy link
Copy Markdown
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

maclover7 mentioned this pull request Feb 4, 2018
Closed
3 tasks
Trott deleted the fix-readline branch January 13, 2022 22:46
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cjihrig cjihrig approved these changes
+1 more reviewer
refack refack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

readline Issues and PRs related to the built-in readline module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL