"Fix" repl race condition #1677

chrisdickinson · 2015-05-11T21:09:15Z

For some value of "fix."

Per #1634, having multiple CLI repls open could lead to inconsistencies in the JSON data being stored. This PR changes the internal repl so that it commits to a temporary file first, then renames it to the target file. @bnoordhuis noted that this will only work if the tmpdir and target dir are on the same mount, but I imagine the worst case is not any worse than conflicting, racing writes, and this should solve the 80% use case.

This PR also re-addresses the problem of non-terminal readline instances corrupting history. Now the internal repl always defaults the written history to [] (vs. trying to set the options in internal/repl as before.)

Additionally, the PR makes it so that repls that fail to load history (for whatever reason) don't crash out – they simply continue without persistent history support.

Finally, this adds the internal modules to the list of files checked by eslint.

Will fix up the commit log once review is over, as I suspect reviewers will have thoughts about the strategy I've taken here!

R=@indutny, @bnoordhuis

chrisdickinson · 2015-05-11T21:09:45Z

src/node.js

-              throw err;
+              console.log('Encountered error with persistent history support.');
+              return cliRepl.createInternalRepl(
+                process.env, false, function(err, repl) {


This seems like an odd style choice, but make jslint didn't complain!

The linter is set up pretty conservative right now. Not sure it has a rule for odd linebreaks, but it seems fine to me there.

silverwind · 2015-05-12T18:41:02Z

lib/internal/repl.js

+  if (!tmpPath) {
+    return ready(new Error('no tmpdir available'));
+  }
+  tmpPath = path.join(tmpPath, process.pid + '-' + 'repl.tmp');


Maybe 'node-repl-' + process.pid to make it clear which process created that file?

silverwind · 2015-06-01T13:32:40Z

Recently had the issue that the REPL failed to spawn because some error when reading the history file, so I'd appreciate this being landed. The linter changes should go, they're not needed anymore.

ghost · 2015-06-05T01:24:37Z

Another thing that would make the REPL history much more reliable is if it didn't save json, only lines of text. A few lines of garbled history is much better than the REPL refusing to open because of history corruption.
I just ran into this bug again and had to go in and fix the history manually.

silverwind · 2015-06-05T02:17:33Z

Totally agree, lines should be stored as lines.

thefourtheye · 2015-06-05T10:26:49Z

lib/internal/repl.js

    useGlobal: true
  };

  if (parseInt(env.NODE_NO_READLINE)) {
    opts.terminal = false;
  }
+  if (parseInt(env.NODE_FORCE_READLINE)) {


Shouldn't we use the radix here?

It shouldn't be a problem in this case – we're just detecting whether the number is "0" or not.

Then we can explicitly do that right? As it is, the intention of this code is not clear, IMO.

chrisdickinson · 2015-06-05T19:07:53Z

~~@seishun~~ (edit: wow, my mind isn't working this morning!) @silverwind @substack Yep, if I could turn back time I would have stored lines instead of JSON. Switching to lines is a bit hairier than it seems now, though. We'd have to include JSON loading as well as line-based loading, include line-based storage, and even then folks who use nvm/nave to switch between versions might get bitten by switching to a JSON-only version. Additionally we have to differentiate the line-based version from the JSON version to avoid false positives. If we do this we should tackle it as a separate issue, and most likely in a major version. Sorry I made it JSON, folks! Hindsight is 20/20.

silverwind · 2015-06-06T09:49:16Z

~~How about parsing the file as lines, but save them as JSON? Might get us at least some fault tolerance.~~

Edit: i think this will get too messy, see below.

silverwind · 2015-06-06T10:26:41Z

Or, you know, just drop JSON support in a major, with an automatic conversion to line-based history in place. People who are using persistent history are early adopters that read changelogs, and I don't think many users will downgrade to previous versions of iojs. nvm is mostly used to switch between 0.10, 0.12 and iojs. If they really hit a iojs version that doesn't support line-based history, they'll know what to do.

Let's do it right, while the feature is still young, imho.

ivan · 2015-06-09T02:30:39Z

Here's one reason you might want to keep storing lines as JSON: users can enter multi-line input in the REPL (e.g. type a { and hit enter), and it would be better to store an entire multi-line input as one entry in the history file. That's easy when you're storing JSON, but harder with plaintext lines (unless you decide to encode newlines as some other control character?). Unfortunately, right now each line of the multi-line input gets its own history entry, but keeping the JSON leaves it easier to fix.

chrisdickinson · 2015-07-08T07:41:01Z

@bnoordhuis Could I get an LGTM on this (or a "this approach will not work?")

brendanashworth · 2015-08-22T01:55:19Z

I don't think this PR is still applicable - https://github.com/chrisdickinson/io.js/commit/46e2d6848ef33461757a4c43939c8b3adbf10784 was replaced by #1686, and the race conditions were fixed in d200932...ed6c249. I'll close this for now, it can be reopened if I'm mistaken.

Fishrock123 · 2015-08-22T02:26:08Z

I didn't actually fix the race conditions. Doing that requires considerably more work. I did make them have minimal impact though.

jasnell · 2015-10-22T02:34:46Z

@chrisdickinson @Fishrock123 ... any progress on this?

Fishrock123 · 2015-10-22T13:46:30Z

@jasnell After some changes that I did it's much less prone to catastrophic error, but it still could work properly. It's pretty complex though.

(It's unlikely that I understand enough about atomic file operations to do this properly.)

jasnell · 2015-11-16T04:30:12Z

@chrisdickinson ... ping :-)

chrisdickinson · 2015-11-16T04:38:46Z

I haven't really seen any complaints about history race conditions since (it might have faded into "general weirdness that folks expect out of history-backed REPLs".) Closing this.

chrisdickinson added 3 commits May 11, 2015 14:00

repl: "fix" race condition between internal repl histories

de5a5f5

style fix

ba80aa1

build: include internal modules in lint

46e2d68

chrisdickinson added the repl Issues and PRs related to the REPL subsystem. label May 11, 2015

chrisdickinson reviewed May 11, 2015
View reviewed changes

@bnoordhuis: use "wx" for writing temp file

e4f0cfc

silverwind reviewed May 12, 2015
View reviewed changes

chrisdickinson added 5 commits May 12, 2015 11:44

add tmpfile cleanup on repl exit; replace TMP with Temp everywhere

9b9dd70

@bnoordhuis: punctuate comments

8752fe1

fix repl error state messaging and close on error

bdf2b9f

note why we use fs.write vs. writeFile

821332b

@silverwind: add "node-" to repl name for clarity

6158e2d

chrisdickinson mentioned this pull request May 12, 2015

tools: make eslint work on subdirectories #1686

Closed

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

thefourtheye reviewed Jun 5, 2015
View reviewed changes

rvagg force-pushed the master branch from 9d21135 to 628a3ab Compare June 25, 2015 09:18

chrisdickinson mentioned this pull request Jul 3, 2015

Collaborator Checkin / 2015-07-02 → 2015-07-09 #2097

Closed

indutny force-pushed the master branch from bffb204 to eb35968 Compare July 22, 2015 21:21

brendanashworth closed this Aug 22, 2015

Fishrock123 reopened this Aug 22, 2015

rvagg force-pushed the master branch from 11c25c2 to ba02bd0 Compare September 6, 2015 11:55

jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015

chrisdickinson closed this Nov 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"Fix" repl race condition #1677

"Fix" repl race condition #1677

chrisdickinson commented May 11, 2015

chrisdickinson May 11, 2015

silverwind May 12, 2015

silverwind May 12, 2015

chrisdickinson May 12, 2015

silverwind commented Jun 1, 2015

ghost commented Jun 5, 2015

silverwind commented Jun 5, 2015

thefourtheye Jun 5, 2015

chrisdickinson Jun 5, 2015

thefourtheye Jun 5, 2015

chrisdickinson commented Jun 5, 2015

silverwind commented Jun 6, 2015

silverwind commented Jun 6, 2015

ivan commented Jun 9, 2015

chrisdickinson commented Jul 8, 2015

brendanashworth commented Aug 22, 2015

Fishrock123 commented Aug 22, 2015

jasnell commented Oct 22, 2015

Fishrock123 commented Oct 22, 2015

jasnell commented Nov 16, 2015

chrisdickinson commented Nov 16, 2015

"Fix" repl race condition #1677

"Fix" repl race condition #1677

Conversation

chrisdickinson commented May 11, 2015

chrisdickinson May 11, 2015

Choose a reason for hiding this comment

silverwind May 12, 2015

Choose a reason for hiding this comment

silverwind May 12, 2015

Choose a reason for hiding this comment

chrisdickinson May 12, 2015

Choose a reason for hiding this comment

silverwind commented Jun 1, 2015

ghost commented Jun 5, 2015

silverwind commented Jun 5, 2015

thefourtheye Jun 5, 2015

Choose a reason for hiding this comment

chrisdickinson Jun 5, 2015

Choose a reason for hiding this comment

thefourtheye Jun 5, 2015

Choose a reason for hiding this comment

chrisdickinson commented Jun 5, 2015

silverwind commented Jun 6, 2015

silverwind commented Jun 6, 2015

ivan commented Jun 9, 2015

chrisdickinson commented Jul 8, 2015

brendanashworth commented Aug 22, 2015

Fishrock123 commented Aug 22, 2015

jasnell commented Oct 22, 2015

Fishrock123 commented Oct 22, 2015

jasnell commented Nov 16, 2015

chrisdickinson commented Nov 16, 2015