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

Fix nrepl port race condition #972

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Fix nrepl port race condition #972

merged 1 commit into from
Jan 27, 2021

Conversation

clyfe
Copy link

@clyfe clyfe commented Jan 21, 2021

What has Changed?

  • Race is: the get-port-from-file code overwrites the port-from-regex with nothing when the nrepl-port file is not yet present at the time is read.
  • Fix: don't get-port-from-file when port already present.

Fixes #901

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
  • Tested the VSIX built locally from the PR (so, after you've submitted the PR).
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • Referenced the issue I am fixing/addressing in a commit message for the pull request.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

  • Made sure the PR is directed at the dev branch (unless reasons).
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and tested it there if so.
  • Read the source changes.
  • Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @cfehse, @bpringe

@PEZ
Copy link
Collaborator

PEZ commented Jan 21, 2021

Ah, great that you found where this happens! It's consistent with the behaviour that this just happens now and then.

However, we can't delete that code. It is used for the Calva Connect command. What you'll need to do is to only execute it conditionally. So if we already have a port, this should not be done, otherwise it should.

CHANGELOG.md Outdated Show resolved Hide resolved
@clyfe
Copy link
Author

clyfe commented Jan 21, 2021

Tested the VSIX built from the PR locally, but not the one off circleci, nor will I do so.

@PEZ
Copy link
Collaborator

PEZ commented Jan 21, 2021

Tested the VSIX built from the PR locally, but not the one off circleci

That's perfectly OK. The point is to make sure the release build works in the sense that the tree shaking and munging hasn't broken things. It is tested just as well with your locally built VSIX:

Copy link
Author

@clyfe clyfe left a comment

Choose a reason for hiding this comment

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

Let's merge.

Copy link
Collaborator

@PEZ PEZ left a comment

Choose a reason for hiding this comment

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

This needs to be addressed before merge.

src/connector.ts Outdated Show resolved Hide resolved
@clyfe clyfe requested a review from PEZ January 27, 2021 12:22
Copy link
Collaborator

@PEZ PEZ left a comment

Choose a reason for hiding this comment

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

Thanks. We're getting there. 😄

Generally (and we should add this to the contribution guidelines) we want fixes like this to be quite surgical. You've found the place for the race condition, you've fixed it. If you see other changes that should be made, better send them in a separate PR.

src/connector.ts Outdated Show resolved Hide resolved
src/connector.ts Outdated Show resolved Hide resolved
src/connector.ts Outdated Show resolved Hide resolved
Race is: the get-port-from-file code overwrites the port-from-regex with nothing when the nrepl-port file is not yet present at the time is read.
Fix: don't get-port-from-file when port already present.
Improved get-port-from-file such that it waits for file to be present with a timeout.

Fixes #901
@clyfe clyfe requested a review from PEZ January 27, 2021 17:55
Copy link
Collaborator

@PEZ PEZ left a comment

Choose a reason for hiding this comment

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

Thanks. Totally appreciate that you helped fix this and followed through.

@PEZ PEZ merged commit 448f3c3 into BetterThanTomorrow:dev Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants