Sitelet https://github.com/nodejs/node/issues/1634
Skip to content

Race condition on history file with multiple REPLs #1634

Closed
Closed
Race condition on history file with multiple REPLs#1634
Assignees
chrisdickinson
Labels
confirmed-bugIssues and PRs for confirmed bugs.replIssues and PRs related to the REPL subsystem.

Description

@rvagg
rvagg
opened on May 6, 2015

via @substack

<substack> found another repl history bug, if you have 2 repls open at once it doesn't lock the file
<substack> but a bit hard to reproduce since they've got to be writing at the same time
<substack> but I already ran into it just doing normal things with 2 repls open at once

Activity

  1. brendanashworth
    added
    replIssues and PRs related to the REPL subsystem.
    on May 6, 2015

  2. chrisdickinson commented on May 7, 2015

    @chrisdickinson
    chrisdickinson
    on May 7, 2015
    Contributor

    Looking into this. I see two possible solutions. If anyone has a better idea, please let me know!

    1. Use tmp files + rename to atomically swap the node history file.
    2. Use fcntl / flock'ing (or a similar mechanism) and make sure that only one cooperating node process at a time is writing to the file.
  3. chrisdickinson
    self-assigned this
    on May 7, 2015

  4. bnoordhuis commented on May 7, 2015

    @bnoordhuis
    bnoordhuis
    on May 7, 2015
    Member

    Option 1 can work as long as the files are on the same mount. Option 2 has portability issues and is unreliable with NFS.

  5. rvagg commented on May 7, 2015

    @rvagg
    rvagg
    on May 7, 2015
    MemberAuthor

    can we see what bash does and copy that?

  6. chrisdickinson commented on May 7, 2015

    @chrisdickinson
    chrisdickinson
    on May 7, 2015
    Contributor

    What about mmap'ing the file & setting a semaphore byte? This might be a bit too heavyweight of a solution, since it would require mmap support be added to libuv.

  7. ivan commented on May 13, 2015

    @ivan
    ivan
    on May 13, 2015
    SponsorContributor

    Just for the record: this happens so frequently not because of racing writes but because both REPLs have the history file open in 'w' mode, one REPL exits and writes some history, and the other REPL exits and writes a shorter history, leaving behind some trailing bytes from the longer history.

  8. Fishrock123
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    on Aug 3, 2015

  9. evanlucas commented on Feb 2, 2016

    @evanlucas
    evanlucas
    on Feb 2, 2016
    Contributor

    It seems like since we moved away from using json, this is no longer prevalent. Do we still need to implement some sort of locking to prevent corruption?

  10. Fishrock123
    added
    good first issueIssues that are suitable for first-time contributors.
    mentor-available
    on Mar 16, 2016

  11. Fishrock123 commented on Mar 16, 2016

    @Fishrock123
    Fishrock123
    on Mar 16, 2016
    Contributor

    Locking would probably be desirable if someone wants to take it up. It would be pretty complex though.

  12. lance commented on May 25, 2016

    @lance
    lance
    on May 25, 2016
    Member

    I started working on a locking implementation for this, but I'm not totally clear on the desired behavior. If multiple REPLs are open should history be interleaved? Or is it last-write-wins behavior?

  13. Fishrock123 commented on May 25, 2016

    @Fishrock123
    Fishrock123
    on May 25, 2016
    Contributor

    @lance either or. I'd prefer interleaved. Most terminals do last-write-is-latest-history.

  14. lance
    removed
    good first issueIssues that are suitable for first-time contributors.
    on Aug 3, 2016

  15. lance commented on Aug 3, 2016

    @lance
    lance
    on Aug 3, 2016
    Member

    Removed good-first-contribution label as I don't think this is the case. At least not if #7005 is any indication.

  16. Trott commented on Jul 9, 2017

    @Trott
    Trott
    on Jul 9, 2017
    Member

    This issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.

  17. Trott
    closed this as completedon Jul 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

confirmed-bugIssues and PRs for confirmed bugs.replIssues and PRs related to the REPL subsystem.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions