repl: don't crash if cannot open history file #3630

evanlucas · 2015-11-02T15:43:58Z

Previously, if we are unable to open the history file, an error would
be thrown. Now, fail and print an error message that we could not open
the history file.

Fixes: #3610

R= @Fishrock123

Fishrock123 · 2015-11-02T15:48:32Z

test/sequential/test-repl-persistent-history.js

+  after: function() {
+    try {
+      fs.rmdirSync(historyPathFail);
+    } catch (err) {}


If this fails the test should fail here, since it will cause a more ambiguous error later.

Good call. Updated

Fishrock123 · 2015-11-02T15:50:37Z

I should note there is also a related but less likely case in the write handler: https://github.com/nodejs/node/blob/master/lib/internal/repl.js#L175

Fishrock123 · 2015-11-02T15:52:14Z

lib/internal/repl.js

+      repl._writeToOutput('\nError: Could not open history file.\n' +
+                          'REPL session history will not be persisted.\n');
+      repl._refreshLine();
+      repl._historyPrev = _replHistoryMessage;


I think this is already handled by https://github.com/nodejs/node/pull/3630/files#diff-ffb9ad69f91d159c68e827d213fb3064R68 ?

It won't hit that code path though if setupHistory is called

Fishrock123 · 2015-11-02T15:55:10Z

This will also happen if the legacy file attempts to load and isn't reachable, see https://github.com/nodejs/node/blob/master/lib/internal/repl.js#L127 -- but I don't think we should change that.

Fishrock123 · 2015-11-02T15:57:09Z

lib/internal/repl.js

-      return ready(err);
+      // Cannot open history file.
+      // Don't crash, just don't persist history.
+      debug('oninit: %s', err.message);


If this is reached we already know what is happening, I suggest moving this after the error message and doing debug(err.stack); like above.

Fishrock123 · 2015-11-02T15:58:33Z

test/sequential/test-repl-persistent-history.js

+  before: function() {
+    try {
+      fs.mkdirSync(historyPathFail);
+    } catch (err) {}


I would also not catch this one, but it is less important, what were you thinking of trying to avoid?

Actually since you're using a different file, catching both will technically be fine.

Well, the test will fail if we catch it now that I look at it. I was trying to avoid it hanging around in the event the test throws somewhere.

I went ahead and removed both

Fishrock123 · 2015-11-02T16:07:08Z

lib/internal/repl.js

+      repl._writeToOutput('\nError: Could not open history file.\n' +
+                          'REPL session history will not be persisted.\n');
+      repl._refreshLine();
+      debug(err.stack);


Nit: space after here for readability

(Just visual separation between logging and closing logic)

Fishrock123 · 2015-11-02T16:08:14Z

LGTM, maybe leave it open a day to see if anyone else comments.

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

Fishrock123 · 2015-11-02T16:11:36Z

lib/internal/repl.js

-      return ready(err);
+      // Cannot open history file.
+      // Don't crash, just don't persist history.
+      repl._writeToOutput('\nError: Could not open history file.\n' +


Maybe "the history file"?

Fishrock123 · 2015-11-02T16:59:21Z

https://ci.nodejs.org/job/node-test-binary-windows/184/RUN_SUBSET=2,VS_VERSION=vs2015,label=win2012r2/tapTestReport/test.tap-235/

not ok 235 test-repl-persistent-history.js
# Failed test # 12
# c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015\label\win2012r2\test\sequential\test-repl-persistent-history.js:248
# throw err;
# ^
# 
# Error: EISDIR: illegal operation on a directory, read
# at Error (native)

evanlucas · 2015-11-02T17:49:33Z

Ok, I'll take a look this afternoon

evanlucas · 2015-11-02T22:16:12Z

ah, apparently, windows does not error on fs.open('<some directory>', 'a+') where OS X and linux do

evanlucas · 2015-11-03T14:01:40Z

Just pushed an update. Let's see if the test passes on Windows this time.

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

evanlucas · 2015-11-03T15:27:45Z

Latest CI with linter fixes: https://ci.nodejs.org/job/node-test-pull-request/667/

Seems like all are happy (minus the PPC ones that keep going offline)

Fishrock123 · 2015-11-03T15:33:09Z

test/sequential/test-repl-persistent-history.js

 // File paths
 const fixtures = path.join(common.testDir, 'fixtures');
 const historyFixturePath = path.join(fixtures, '.node_repl_history');
 const historyPath = path.join(common.tmpDir, '.fixture_copy_repl_history');
+const historyPathFail = path.join(common.tmpDir, '.node_repl\u0000_history');


Just an invalid path instead?

What if we do have a invalid mode file then? What if the file is read-only?

I guess at that point we can just not care, since we make the file with specific perms and then it's probably at the hands of the user.

This was more to test when fs.open fails, which is the same thing that will happen when we don't have permission to open the file in the first place.

Fishrock123 · 2015-11-03T15:38:54Z

LGTM

Previously, if we are unable to open the history file, an error would be thrown. Now, print an error message that we could not open the history file, but don't fail. Fixes: nodejs#3610 PR-URL: nodejs#3630 Reviewed-By: Jeremiah Senkpiel <[email protected]>

evanlucas · 2015-11-03T15:56:26Z

Thanks! Landed in a4a0efc

Previously, if we are unable to open the history file, an error would be thrown. Now, print an error message that we could not open the history file, but don't fail. Fixes: #3610 PR-URL: #3630 Reviewed-By: Jeremiah Senkpiel <[email protected]>

Notable changes: * **child_process**: child.send() now properly returns a boolean like the docs suggest (Rich Trott) nodejs#3577. * **http_parser**: update http-parser to 2.6.0 from 2.5.0 (James M Snell) nodejs#3569. - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`, `REBIND`, `UNBIND`. - Also added ACL and IPv6 Zone ID support. * **npm**: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner) nodejs#3685. - See the release notes for https://github.com/npm/npm/releases/tag/v3.3.7, https://github.com/npm/npm/releases/tag/v3.3.8, https://github.com/npm/npm/releases/tag/v3.3.9, https://github.com/npm/npm/releases/tag/v3.3.10, https://github.com/npm/npm/releases/tag/v3.3.11, and https://github.com/npm/npm/releases/tag/v3.3.12 for more details. * **repl**: The REPL no longer crashes if the persistent history file cannot be opened (Evan Lucas) nodejs#3630.

Previously, if we are unable to open the history file, an error would be thrown. Now, print an error message that we could not open the history file, but don't fail. Fixes: #3610 PR-URL: #3630 Reviewed-By: Jeremiah Senkpiel <[email protected]>

MylesBorins · 2015-11-16T23:55:14Z

landed in v4.x-staging as 486d287

Notable changes: * **child_process**: child.send() now properly returns a boolean like the docs suggest (Rich Trott) nodejs#3577. * **http_parser**: update http-parser to 2.6.0 from 2.5.0 (James M Snell) nodejs#3569. - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`, `REBIND`, `UNBIND`. - Also added ACL and IPv6 Zone ID support. * **npm**: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner) nodejs#3685. - See the release notes for https://github.com/npm/npm/releases/tag/v3.3.7, https://github.com/npm/npm/releases/tag/v3.3.8, https://github.com/npm/npm/releases/tag/v3.3.9, https://github.com/npm/npm/releases/tag/v3.3.10, https://github.com/npm/npm/releases/tag/v3.3.11, and https://github.com/npm/npm/releases/tag/v3.3.12 for more details. * **repl**: The REPL no longer crashes if the persistent history file cannot be opened (Evan Lucas) nodejs#3630. PR-URL: nodejs#3736

Notable changes: * buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) #3767. - This makes the behavior match what the docs suggest. * child_process: `child.send()` now properly returns a boolean like the docs suggest (Rich Trott) #3577. * doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) #3662. * http_parser: update http-parser to 2.6.0 from 2.5.0 (James M Snell) #3569. - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`, `REBIND`, `UNBIND`. - Also added ACL and IPv6 Zone ID support. * npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner) #3685. - See the release notes for https://github.com/npm/npm/releases/tag/v3.3.7, https://github.com/npm/npm/releases/tag/v3.3.8, https://github.com/npm/npm/releases/tag/v3.3.9, https://github.com/npm/npm/releases/tag/v3.3.10, https://github.com/npm/npm/releases/tag/v3.3.11, and https://github.com/npm/npm/releases/tag/v3.3.12 for more details. * repl: The REPL no longer crashes if the persistent history file cannot be opened (Evan Lucas) #3630. * tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) #3755. * v8: Added some more useful post-mortem data (Fedor Indutny) #3779. PR-URL: #3736

Previously, if we are unable to open the history file, an error would be thrown. Now, print an error message that we could not open the history file, but don't fail. Fixes: #3610 PR-URL: #3630 Reviewed-By: Jeremiah Senkpiel <[email protected]>

Notable changes: * buffer: The `noAssert` option for many buffer functions will now silently drop invalid write values rather than crashing (P.S.V.R) nodejs/node#3767. - This makes the behavior match what the docs suggest. * child_process: `child.send()` now properly returns a boolean like the docs suggest (Rich Trott) nodejs/node#3577. * doc: All of the API docs have been re-ordered so as to read in alphabetical order (Tristian Flanagan) nodejs/node#3662. * http_parser: update http-parser to 2.6.0 from 2.5.0 (James M Snell) nodejs/node#3569. - Now supports the following HTTP methods: `LINK`, `UNLINK`, `BIND`, `REBIND`, `UNBIND`. - Also added ACL and IPv6 Zone ID support. * npm: upgrade npm to 3.3.12 from v3.3.6 (Rebecca Turner) nodejs/node#3685. - See the release notes for https://github.com/npm/npm/releases/tag/v3.3.7, https://github.com/npm/npm/releases/tag/v3.3.8, https://github.com/npm/npm/releases/tag/v3.3.9, https://github.com/npm/npm/releases/tag/v3.3.10, https://github.com/npm/npm/releases/tag/v3.3.11, and https://github.com/npm/npm/releases/tag/v3.3.12 for more details. * repl: The REPL no longer crashes if the persistent history file cannot be opened (Evan Lucas) nodejs/node#3630. * tls: The default `sessionIdContext` now uses SHA1 in FIPS mode rather than MD5 (Stefan Budeanu) nodejs/node#3755. * v8: Added some more useful post-mortem data (Fedor Indutny) nodejs/node#3779. PR-URL: nodejs/node#3736

evanlucas added the repl Issues and PRs related to the REPL subsystem. label Nov 2, 2015