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

repl: fix/improve persistent repl history #2356

Merged
merged 2 commits into from
Oct 19, 2015

Conversation

Fishrock123
Copy link
Contributor

Fix the history limiting to take the most recent. History additions are unshifted in readline.

Now tests the saved repl history size limiting.

Cleans up the test a bunch in the process, and also adds some debug so you can figure out which test case failed if it fails. Let me know if there is a better way to do that.

@Fishrock123 Fishrock123 added the repl Issues and PRs related to the REPL subsystem. label Aug 12, 2015
@evanlucas
Copy link
Contributor

Still failing for me.

Failed test # 8

/Users/evan/dev/code/forks/io.js/test/sequential/test-repl-persistent-history.js:197
          throw err;
          ^
AssertionError: '> \'=^.^=\'' === '> \'42\''
    at Writable.write [as _write] (/Users/evan/dev/code/forks/io.js/test/sequential/test-repl-persistent-history.js:194:18)
    at doWrite (_stream_writable.js:292:12)
    at writeOrBuffer (_stream_writable.js:278:5)
    at Writable.write (_stream_writable.js:207:11)
    at REPLServer._writeToOutput (readline.js:219:17)
    at REPLServer.Interface._refreshLine (readline.js:259:8)
    at REPLServer.Interface._historyPrev (readline.js:572:10)
    at REPLServer.Interface._ttyWrite (readline.js:858:14)
    at ActionStream.onkeypress (readline.js:105:10)
    at emitTwo (events.js:87:13)

@evanlucas
Copy link
Contributor

Yep, that fixes it for me. We should probably run CI though to be safe.

@Fishrock123
Copy link
Contributor Author

@evanlucas
Copy link
Contributor

LGTM if the CI is happy

@Fishrock123
Copy link
Contributor Author

Hmm, windows isn't liking the cleanup, maybe I'll just have to refresh the tmpdir.

not ok 875 - test-repl-persistent-history.js
# c:\workspace\node-test-commit-windows\nodes\win2012r2\test\sequential\test-repl-persistent-history.js:213
# if (err) throw err;
# ^
# 
# Error: EPERM: operation not permitted, open 'c:\workspace\node-test-commit-windows\nodes\win2012r2\test\tmp.0\.node_repl_history'
# at Error (native)

@Fishrock123
Copy link
Contributor Author

@Fishrock123
Copy link
Contributor Author

Uh, ok.. Hrmmm. New windows (and ARM) failure from common.refreshTmpDir()...

not ok 875 - test-repl-persistent-history.js
# c:\workspace\node-test-commit-windows\nodes\win2012r2\test\common.js:34
# throw e;
# ^
# 
# Error: ENOTEMPTY: directory not empty, rmdir 'c:\workspace\node-test-commit-windows\nodes\win2012r2\test\tmp.0'
# at Error (native)
# at Object.fs.rmdirSync (fs.js:763:18)
# at rmdirSync (c:\workspace\node-test-commit-windows\nodes\win2012r2\test\common.js:49:10)
# at rimrafSync (c:\workspace\node-test-commit-windows\nodes\win2012r2\test\common.js:25:7)
# at Object.exports.refreshTmpDir (c:\workspace\node-test-commit-windows\nodes\win2012r2\test\common.js:55:3)
# at REPLServer.<anonymous> (c:\workspace\node-test-commit-windows\nodes\win2012r2\test\sequential\test-repl-persistent-history.js:211:16)
# at emitNone (events.js:72:20)
# at REPLServer.emit (events.js:166:7)
# at REPLServer.Interface.close (readline.js:285:8)
# at REPLServer.replClose [as close] (repl.js:486:29)

@Fishrock123
Copy link
Contributor Author

I think it needed to wait for I/O to close. Fingers crossed...

CI again: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/88/

@evanlucas
Copy link
Contributor

CI still not happy on Windows :[

not ok 875 - test-repl-persistent-history.js
# c:\workspace\node-test-commit-windows\nodes\win2008r2\test\common.js:34
# throw e;
# ^
# 
# Error: ENOTEMPTY: directory not empty, rmdir 'c:\workspace\node-test-commit-windows\nodes\win2008r2\test\tmp.0'
# at Error (native)
# at Object.fs.rmdirSync (fs.js:763:18)
# at rmdirSync (c:\workspace\node-test-commit-windows\nodes\win2008r2\test\common.js:49:10)
# at rimrafSync (c:\workspace\node-test-commit-windows\nodes\win2008r2\test\common.js:25:7)
# at Object.exports.refreshTmpDir (c:\workspace\node-test-commit-windows\nodes\win2008r2\test\common.js:55:3)
# at Immediate.setupTest (c:\workspace\node-test-commit-windows\nodes\win2008r2\test\sequential\test-repl-persistent-history.js:163:23)
# at Immediate.immediate._onImmediate (timers.js:423:18)
# at processImmediate [as _immediateCallback] (timers.js:371:17)

@Fishrock123
Copy link
Contributor Author

I'm not sure what's up with that.

Somehow this is failing: https://github.com/nodejs/node/blob/master/test/common.js#L15-L57

Specifically: https://github.com/nodejs/node/blob/master/test/common.js#L45-L48

I thought it might have been if the history file was written to after it starts clearing the files but I don't think that is possible...

@silverwind
Copy link
Contributor

Might be a Windows quirk: isaacs/rimraf#72

I think it must be intermittent, but we should still attempt an workaround later. Maybe give the CI a few more runs?

@evanlucas
Copy link
Contributor

@Fishrock123 any update on this?

@Fishrock123
Copy link
Contributor Author

@evanlucas on the back burner right now. I think It can be fixed by replacing the refreshTmpDir with a function that checks if the file exists with a stat, and then unlinks if it does.

@silverwind
Copy link
Contributor

I think this can be worked around by running the rmDirSync a few times. I dealt with these Windows issues before and I think running 10-20 times usually does the job. It's an ugly hack indeed.

@Fishrock123
Copy link
Contributor Author

Hopefully this last patch fixed those stupid windows issues. https://jenkins-iojs.nodesource.com/job/node-test-pull-request/182/

@Fishrock123
Copy link
Contributor Author

Hmm, the rebase failed on that. here's a manual run: https://jenkins-iojs.nodesource.com/job/node-test-commit/339/

@Fishrock123
Copy link
Contributor Author

Ugh. EPERM. Whatever that means in this context.

@silverwind
Copy link
Contributor

Might as well install a full rimraf and be done with it, I know that it can handle EPERM at least.

@Fishrock123
Copy link
Contributor Author

@Fishrock123
Copy link
Contributor Author

Ok this EPERM isn't coming from the unlinkSync though. I think it's coming from fs.open in internal/repl...?

joaocgreis added a commit to JaneaSystems/node that referenced this pull request Sep 2, 2015
This test is already being investigated, but until a solution is found
it should be marked flaky.

Ref: nodejs#2319
Ref: nodejs#2356
joaocgreis added a commit that referenced this pull request Sep 2, 2015
This test is already being investigated, but until a solution is found
it should be marked flaky.

Ref: #2319
Ref: #2356

PR-URL: #2659
Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>
joaocgreis added a commit that referenced this pull request Sep 3, 2015
This test is already being investigated, but until a solution is found
it should be marked flaky.

Ref: #2319
Ref: #2356

PR-URL: #2659
Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 3, 2015
This test is already being investigated, but until a solution is found
it should be marked flaky.

Ref: nodejs#2319
Ref: nodejs#2356

PR-URL: nodejs#2659
Reviewed-By: orangemocha - Alexis Campailla <orangemocha@nodejs.org>
@Fishrock123
Copy link
Contributor Author

cc @nodejs/collaborators PTAL at the last commit

// Ensure everything that we expected was output
assert.strictEqual(expected.length, 0);
setImmediate(runTest);
onClose();
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@silverwind @evanlucas can I get a last LGTM on this? this works without any even minor breaking changes now.

@evanlucas
Copy link
Contributor

Ok, just pulled down and tested. I've run the tests hundreds of times now and haven't had them fail.

LGTM

- Now cleans up the history file unless told otherwise.
- Now also logs which test case failed.
- Waits for flush after repl close if necessary.

Fixes: nodejs#2319
PR-URL: nodejs#2356
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed By: Evan Lucas <evanlucas@me.com>
Previously the wrong end of the history was limited on load.

PR-URL: nodejs#2356
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed By: Evan Lucas <evanlucas@me.com>
@Fishrock123 Fishrock123 merged commit 73b7e05 into nodejs:master Oct 19, 2015
@Fishrock123
Copy link
Contributor Author

Thanks, landed in d8db757...73b7e05

@nodejs/lts this is something we want in v4.x.

Fishrock123 added a commit that referenced this pull request Oct 21, 2015
- Now cleans up the history file unless told otherwise.
- Now also logs which test case failed.
- Waits for flush after repl close if necessary.

Fixes: #2319
PR-URL: #2356
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed By: Evan Lucas <evanlucas@me.com>
Fishrock123 added a commit that referenced this pull request Oct 21, 2015
Previously the wrong end of the history was limited on load.

PR-URL: #2356
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed By: Evan Lucas <evanlucas@me.com>
@rvagg rvagg mentioned this pull request Oct 21, 2015
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Oct 22, 2015
Emitting 'close' before the history has flushed is somewhat incorrect
and rather confusing.

This also makes the 'close' event always asynchronous for consistency.

Refs: nodejs#2356
PR-URL: nodejs#3435
Reviewed By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Fishrock123 added a commit that referenced this pull request Oct 22, 2015
Emitting 'close' before the history has flushed is somewhat incorrect
and rather confusing.

This also makes the 'close' event always asynchronous for consistency.

Refs: #2356
PR-URL: #3435
Reviewed By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Fishrock123 added a commit that referenced this pull request Oct 26, 2015
- Now cleans up the history file unless told otherwise.
- Now also logs which test case failed.
- Waits for flush after repl close if necessary.

Fixes: #2319
PR-URL: #2356
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed By: Evan Lucas <evanlucas@me.com>
Fishrock123 added a commit that referenced this pull request Oct 26, 2015
Previously the wrong end of the history was limited on load.

PR-URL: #2356
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed By: Evan Lucas <evanlucas@me.com>
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

Landed in v4.x-staging in 9fa8c32 and 594e8e7

Fishrock123 added a commit that referenced this pull request Oct 29, 2015
- Now cleans up the history file unless told otherwise.
- Now also logs which test case failed.
- Waits for flush after repl close if necessary.

Fixes: #2319
PR-URL: #2356
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed By: Evan Lucas <evanlucas@me.com>
Fishrock123 added a commit that referenced this pull request Oct 29, 2015
Previously the wrong end of the history was limited on load.

PR-URL: #2356
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed By: Evan Lucas <evanlucas@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants