{"action":"created","comment":{"url":"https://api.github.com/repos/nodejs/node/pulls/comments/64993992","id":64993992,"diff_hunk":"@@ -192,31 +188,49 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {\n     timer = setTimeout(flushHistory, kDebounceHistoryMS);\n   }\n \n+  const historyLockFile = `${historyPath}.LCK`;\n   function flushHistory() {\n     timer = null;\n     if (writing) {\n       pending = true;\n       return;\n     }\n-    writing = true;\n-    const historyData = repl.history.join(os.EOL);\n-    fs.write(repl._historyHandle, historyData, 0, 'utf8', onwritten);\n-  }\n-\n-  function onwritten(err, data) {\n-    writing = false;\n-    if (pending) {\n-      pending = false;\n-      online();\n-    } else {\n-      repl._flushing = Boolean(timer);\n-      if (!repl._flushing) {\n-        repl.emit('flushHistory');\n+    // Ref: https://github.com/nodejs/node/issues/1634\n+    // opening a file for exclusive write should be atomic\n+    // on non-networked POSIX systems.\n+    fs.open(historyLockFile, 'wx', (err, fd) => {\n+      if (err && err.code === 'EEXIST') {\n+        pending = true;\n+        repl.resume();\n+        return;\n       }\n-    }\n+      writing = true;\n+      const lastLine = repl.history[0] + '\\n';\n+      fs.write(repl._historyHandle, lastLine, NaN, 'utf8', onwritten(fd));\n+    });\n   }\n-}\n \n+  function onwritten(fd) {\n+    return (err, data) => {\n+      writing = false;\n+      if (pending) {\n+        pending = false;\n+        online();\n+      } else {\n+        repl._flushing = Boolean(timer);\n+        if (!repl._flushing) {\n+          repl.emit('flushHistory');\n+        }\n+      }\n+      unlockHistoryFile(fd);","path":"lib/internal/repl.js","position":89,"original_position":89,"commit_id":"30e2048cc6b9af717023e4d9a48136b4b35e0d1f","original_commit_id":"30e2048cc6b9af717023e4d9a48136b4b35e0d1f","user":{"login":"lance","id":15952,"avatar_url":"https://avatars.githubusercontent.com/u/15952?v=3","gravatar_id":"","url":"https://api.github.com/users/lance","html_url":"https://github.com/lance","followers_url":"https://api.github.com/users/lance/followers","following_url":"https://api.github.com/users/lance/following{/other_user}","gists_url":"https://api.github.com/users/lance/gists{/gist_id}","starred_url":"https://api.github.com/users/lance/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lance/subscriptions","organizations_url":"https://api.github.com/users/lance/orgs","repos_url":"https://api.github.com/users/lance/repos","events_url":"https://api.github.com/users/lance/events{/privacy}","received_events_url":"https://api.github.com/users/lance/received_events","type":"User","site_admin":false},"body":"Hmm - I can only reproduce this when there is already a stray `.node_repl_history.LCK` file at start up, and the history file is never written to. When the `REPLServer` is closing it adds a listener for `flushHistory` [here](https://github.com/nodejs/node/blob/master/lib/repl.js#L527-L539) which never gets called, and so the final `Interface#close` is never called.\r\n\r\nI can add a check for the existence of the `.LCK` file at startup and then remove it if necessary. There are maybe a couple of ways this could happen.\r\n\r\n  * Notify the user. \"Hey! There's a lock file, do you want me to delete it?\". Probably the safest, but pretty intrusive and kind of looks like its underwear is showing.\r\n  * Set one or more brief timeouts to check/recheck for the existence before deleting. This doesn't eliminate the possibility of a race condition, but it definitely minimizes it.","created_at":"2016-05-28T20:35:33Z","updated_at":"2016-05-28T20:35:33Z","html_url":"https://github.com/nodejs/node/pull/7005#discussion_r64993992","pull_request_url":"https://api.github.com/repos/nodejs/node/pulls/7005","_links":{"self":{"href":"https://api.github.com/repos/nodejs/node/pulls/comments/64993992"},"html":{"href":"https://github.com/nodejs/node/pull/7005#discussion_r64993992"},"pull_request":{"href":"https://api.github.com/repos/nodejs/node/pulls/7005"}}},"pull_request":{"url":"https://api.github.com/repos/nodejs/node/pulls/7005","id":71623165,"html_url":"https://github.com/nodejs/node/pull/7005","diff_url":"https://github.com/nodejs/node/pull/7005.diff","patch_url":"https://github.com/nodejs/node/pull/7005.patch","issue_url":"https://api.github.com/repos/nodejs/node/issues/7005","number":7005,"state":"open","locked":false,"title":"repl: History file locking and other improvements","user":{"login":"lance","id":15952,"avatar_url":"https://avatars.githubusercontent.com/u/15952?v=3","gravatar_id":"","url":"https://api.github.com/users/lance","html_url":"https://github.com/lance","followers_url":"https://api.github.com/users/lance/followers","following_url":"https://api.github.com/users/lance/following{/other_user}","gists_url":"https://api.github.com/users/lance/gists{/gist_id}","starred_url":"https://api.github.com/users/lance/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lance/subscriptions","organizations_url":"https://api.github.com/users/lance/orgs","repos_url":"https://api.github.com/users/lance/repos","events_url":"https://api.github.com/users/lance/events{/privacy}","received_events_url":"https://api.github.com/users/lance/received_events","type":"User","site_admin":false},"body":"##### Checklist\r\n- [X] tests and code linting passes\r\n- [ ] a test and/or benchmark is included\r\n- [ ] documentation is changed or added\r\n- [X] the commit message follows commit guidelines\r\n\r\n##### Affected core subsystem(s)\r\nrepl\r\n\r\n##### Description of change\r\n\r\nThere are several open issues regarding the repl's history file, and it\r\nturns out that these issues are all somewhat related.\r\n\r\nThe code changes here started as basic file locking to prevent\r\nsimultaneous writes on the history file. This led to the realization\r\nthat the history file was rewritten in its entirety for each expression\r\nevaluated in the repl. This, of course, meant that interleaving writes\r\nbetween multiple processes wasn't really possible. To make that happen\r\nthe file needs to be open for writing with `'a'` instead of `'w'`.\r\n\r\nAs it turns out, changing this call to `fs.open()` to use `'a'` also\r\nshould fix a problem with `.node_repl_history` being hidden on Windows\r\nsystems (see https://github.com/nodejs/node/issues/5261). Although, I do\r\nnot have a Windows box to test against.\r\n\r\nBenefits:\r\n\r\n * Don't rewrite the entire history file for every evaluated expression.\r\n * Don't lose all but the last NODE_HISTORY_FILE_SIZE number of lines on\r\n   repl startup.\r\n * No history file corruption from simultaneous writes by multiple\r\n   processes.\r\n * History is interleaved for multiple processes in a single history.\r\n * In theory fixes a known issue on Windows.\r\n\r\nRef: https://github.com/nodejs/node/issues/5261\r\nRef: https://github.com/nodejs/node/issues/1634\r\nRef: https://github.com/nodejs/node/issues/3928\r\nRef: https://github.com/nodejs/node/pull/4313\r\n\r\nrepl: Add file locking for repl history file.\r\n\r\nAdds a .node_repl_history.LCK file to prevent concurrent writes to the\r\nNODE_REPL_HISTORY file from different processes.\r\n\r\nrepl: Adjust tests for reverse order of log file.\r\n\r\nSince the repl now stores history as most recent last, the tests have\r\nbeen adjusted to account for that.\r\n\r\n##### Remaining work\r\nExisting tests have been adjusted to account for the fact that the repl now appends log file data. But no tests have been written yet which exercise the interleaving of repl history from two separate instances. I'm working on it, but it has been a little tricky. I wanted to get this PR submitted as a placeholder, and if the direction I'm taking is OK, then I'll finish up the test. \r\n\r\nI don't think this requires a documentation change, because these are mostly bug fixes and improvements. But there is the fact that the history file is now reversed. It's not currently documented how the history file should look, and I think this is probably not necessary to document. But I wasn't sure, so I left the checkbox there and unchecked.","created_at":"2016-05-26T20:56:33Z","updated_at":"2016-05-28T20:35:34Z","closed_at":null,"merged_at":null,"merge_commit_sha":"8c7d5aa62197a0e8f17965a183f5021cc2f1fa2c","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/nodejs/node/pulls/7005/commits","review_comments_url":"https://api.github.com/repos/nodejs/node/pulls/7005/comments","review_comment_url":"https://api.github.com/repos/nodejs/node/pulls/comments{/number}","comments_url":"https://api.github.com/repos/nodejs/node/issues/7005/comments","statuses_url":"https://api.github.com/repos/nodejs/node/statuses/30e2048cc6b9af717023e4d9a48136b4b35e0d1f","head":{"label":"lance:repl-history-file","ref":"repl-history-file","sha":"30e2048cc6b9af717023e4d9a48136b4b35e0d1f","user":{"login":"lance","id":15952,"avatar_url":"https://avatars.githubusercontent.com/u/15952?v=3","gravatar_id":"","url":"https://api.github.com/users/lance","html_url":"https://github.com/lance","followers_url":"https://api.github.com/users/lance/followers","following_url":"https://api.github.com/users/lance/following{/other_user}","gists_url":"https://api.github.com/users/lance/gists{/gist_id}","starred_url":"https://api.github.com/users/lance/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lance/subscriptions","organizations_url":"https://api.github.com/users/lance/orgs","repos_url":"https://api.github.com/users/lance/repos","events_url":"https://api.github.com/users/lance/events{/privacy}","received_events_url":"https://api.github.com/users/lance/received_events","type":"User","site_admin":false},"repo":{"id":52987437,"name":"node","full_name":"lance/node","owner":{"login":"lance","id":15952,"avatar_url":"https://avatars.githubusercontent.com/u/15952?v=3","gravatar_id":"","url":"https://api.github.com/users/lance","html_url":"https://github.com/lance","followers_url":"https://api.github.com/users/lance/followers","following_url":"https://api.github.com/users/lance/following{/other_user}","gists_url":"https://api.github.com/users/lance/gists{/gist_id}","starred_url":"https://api.github.com/users/lance/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lance/subscriptions","organizations_url":"https://api.github.com/users/lance/orgs","repos_url":"https://api.github.com/users/lance/repos","events_url":"https://api.github.com/users/lance/events{/privacy}","received_events_url":"https://api.github.com/users/lance/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/lance/node","description":"Node.js JavaScript runtime :sparkles::turtle::rocket::sparkles:","fork":true,"url":"https://api.github.com/repos/lance/node","forks_url":"https://api.github.com/repos/lance/node/forks","keys_url":"https://api.github.com/repos/lance/node/keys{/key_id}","collaborators_url":"https://api.github.com/repos/lance/node/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/lance/node/teams","hooks_url":"https://api.github.com/repos/lance/node/hooks","issue_events_url":"https://api.github.com/repos/lance/node/issues/events{/number}","events_url":"https://api.github.com/repos/lance/node/events","assignees_url":"https://api.github.com/repos/lance/node/assignees{/user}","branches_url":"https://api.github.com/repos/lance/node/branches{/branch}","tags_url":"https://api.github.com/repos/lance/node/tags","blobs_url":"https://api.github.com/repos/lance/node/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/lance/node/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/lance/node/git/refs{/sha}","trees_url":"https://api.github.com/repos/lance/node/git/trees{/sha}","statuses_url":"https://api.github.com/repos/lance/node/statuses/{sha}","languages_url":"https://api.github.com/repos/lance/node/languages","stargazers_url":"https://api.github.com/repos/lance/node/stargazers","contributors_url":"https://api.github.com/repos/lance/node/contributors","subscribers_url":"https://api.github.com/repos/lance/node/subscribers","subscription_url":"https://api.github.com/repos/lance/node/subscription","commits_url":"https://api.github.com/repos/lance/node/commits{/sha}","git_commits_url":"https://api.github.com/repos/lance/node/git/commits{/sha}","comments_url":"https://api.github.com/repos/lance/node/comments{/number}","issue_comment_url":"https://api.github.com/repos/lance/node/issues/comments{/number}","contents_url":"https://api.github.com/repos/lance/node/contents/{+path}","compare_url":"https://api.github.com/repos/lance/node/compare/{base}...{head}","merges_url":"https://api.github.com/repos/lance/node/merges","archive_url":"https://api.github.com/repos/lance/node/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/lance/node/downloads","issues_url":"https://api.github.com/repos/lance/node/issues{/number}","pulls_url":"https://api.github.com/repos/lance/node/pulls{/number}","milestones_url":"https://api.github.com/repos/lance/node/milestones{/number}","notifications_url":"https://api.github.com/repos/lance/node/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/lance/node/labels{/name}","releases_url":"https://api.github.com/repos/lance/node/releases{/id}","deployments_url":"https://api.github.com/repos/lance/node/deployments","created_at":"2016-03-02T18:47:13Z","updated_at":"2016-03-02T18:47:24Z","pushed_at":"2016-05-26T20:50:25Z","git_url":"git://github.com/lance/node.git","ssh_url":"git@github.com:lance/node.git","clone_url":"https://github.com/lance/node.git","svn_url":"https://github.com/lance/node","homepage":"https://nodejs.org","size":207581,"stargazers_count":0,"watchers_count":0,"language":"JavaScript","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"nodejs:master","ref":"master","sha":"da12b10005e9ab4e52feb4101ac0ba373d290ad6","user":{"login":"nodejs","id":9950313,"avatar_url":"https://avatars.githubusercontent.com/u/9950313?v=3","gravatar_id":"","url":"https://api.github.com/users/nodejs","html_url":"https://github.com/nodejs","followers_url":"https://api.github.com/users/nodejs/followers","following_url":"https://api.github.com/users/nodejs/following{/other_user}","gists_url":"https://api.github.com/users/nodejs/gists{/gist_id}","starred_url":"https://api.github.com/users/nodejs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nodejs/subscriptions","organizations_url":"https://api.github.com/users/nodejs/orgs","repos_url":"https://api.github.com/users/nodejs/repos","events_url":"https://api.github.com/users/nodejs/events{/privacy}","received_events_url":"https://api.github.com/users/nodejs/received_events","type":"Organization","site_admin":false},"repo":{"id":27193779,"name":"node","full_name":"nodejs/node","owner":{"login":"nodejs","id":9950313,"avatar_url":"https://avatars.githubusercontent.com/u/9950313?v=3","gravatar_id":"","url":"https://api.github.com/users/nodejs","html_url":"https://github.com/nodejs","followers_url":"https://api.github.com/users/nodejs/followers","following_url":"https://api.github.com/users/nodejs/following{/other_user}","gists_url":"https://api.github.com/users/nodejs/gists{/gist_id}","starred_url":"https://api.github.com/users/nodejs/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nodejs/subscriptions","organizations_url":"https://api.github.com/users/nodejs/orgs","repos_url":"https://api.github.com/users/nodejs/repos","events_url":"https://api.github.com/users/nodejs/events{/privacy}","received_events_url":"https://api.github.com/users/nodejs/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/nodejs/node","description":"Node.js JavaScript runtime :sparkles::turtle::rocket::sparkles:","fork":false,"url":"https://api.github.com/repos/nodejs/node","forks_url":"https://api.github.com/repos/nodejs/node/forks","keys_url":"https://api.github.com/repos/nodejs/node/keys{/key_id}","collaborators_url":"https://api.github.com/repos/nodejs/node/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/nodejs/node/teams","hooks_url":"https://api.github.com/repos/nodejs/node/hooks","issue_events_url":"https://api.github.com/repos/nodejs/node/issues/events{/number}","events_url":"https://api.github.com/repos/nodejs/node/events","assignees_url":"https://api.github.com/repos/nodejs/node/assignees{/user}","branches_url":"https://api.github.com/repos/nodejs/node/branches{/branch}","tags_url":"https://api.github.com/repos/nodejs/node/tags","blobs_url":"https://api.github.com/repos/nodejs/node/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/nodejs/node/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/nodejs/node/git/refs{/sha}","trees_url":"https://api.github.com/repos/nodejs/node/git/trees{/sha}","statuses_url":"https://api.github.com/repos/nodejs/node/statuses/{sha}","languages_url":"https://api.github.com/repos/nodejs/node/languages","stargazers_url":"https://api.github.com/repos/nodejs/node/stargazers","contributors_url":"https://api.github.com/repos/nodejs/node/contributors","subscribers_url":"https://api.github.com/repos/nodejs/node/subscribers","subscription_url":"https://api.github.com/repos/nodejs/node/subscription","commits_url":"https://api.github.com/repos/nodejs/node/commits{/sha}","git_commits_url":"https://api.github.com/repos/nodejs/node/git/commits{/sha}","comments_url":"https://api.github.com/repos/nodejs/node/comments{/number}","issue_comment_url":"https://api.github.com/repos/nodejs/node/issues/comments{/number}","contents_url":"https://api.github.com/repos/nodejs/node/contents/{+path}","compare_url":"https://api.github.com/repos/nodejs/node/compare/{base}...{head}","merges_url":"https://api.github.com/repos/nodejs/node/merges","archive_url":"https://api.github.com/repos/nodejs/node/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/nodejs/node/downloads","issues_url":"https://api.github.com/repos/nodejs/node/issues{/number}","pulls_url":"https://api.github.com/repos/nodejs/node/pulls{/number}","milestones_url":"https://api.github.com/repos/nodejs/node/milestones{/number}","notifications_url":"https://api.github.com/repos/nodejs/node/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/nodejs/node/labels{/name}","releases_url":"https://api.github.com/repos/nodejs/node/releases{/id}","deployments_url":"https://api.github.com/repos/nodejs/node/deployments","created_at":"2014-11-26T19:57:11Z","updated_at":"2016-05-28T20:30:10Z","pushed_at":"2016-05-28T20:35:25Z","git_url":"git://github.com/nodejs/node.git","ssh_url":"git@github.com:nodejs/node.git","clone_url":"https://github.com/nodejs/node.git","svn_url":"https://github.com/nodejs/node","homepage":"https://nodejs.org","size":278687,"stargazers_count":23567,"watchers_count":23567,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":3385,"mirror_url":null,"open_issues_count":685,"forks":3385,"open_issues":685,"watchers":23567,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/nodejs/node/pulls/7005"},"html":{"href":"https://github.com/nodejs/node/pull/7005"},"issue":{"href":"https://api.github.com/repos/nodejs/node/issues/7005"},"comments":{"href":"https://api.github.com/repos/nodejs/node/issues/7005/comments"},"review_comments":{"href":"https://api.github.com/repos/nodejs/node/pulls/7005/comments"},"review_comment":{"href":"https://api.github.com/repos/nodejs/node/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/nodejs/node/pulls/7005/commits"},"statuses":{"href":"https://api.github.com/repos/nodejs/node/statuses/30e2048cc6b9af717023e4d9a48136b4b35e0d1f"}}}}
{"actor":{"display_login":"lance"}}