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

Jack-in prompting for nrepl port since terminal jack-in change #901

Closed
bpringe opened this issue Dec 29, 2020 · 11 comments
Closed

Jack-in prompting for nrepl port since terminal jack-in change #901

bpringe opened this issue Dec 29, 2020 · 11 comments
Labels

Comments

@bpringe
Copy link
Member

bpringe commented Dec 29, 2020

Since the recent jack-in change, I'm getting a prompt for the nrepl port at jack-in. This doesn't happen every time though. Maybe it's a race condition related to the .nrepl-port file that's created.

image

@bpringe
Copy link
Member Author

bpringe commented Dec 29, 2020

This appears to be working fine too now. 🙄 See comments of #903.

@bpringe bpringe closed this as completed Dec 29, 2020
@bpringe
Copy link
Member Author

bpringe commented Dec 30, 2020

This is actually still happening intermittently, at least with leiningen, on linux.

@bpringe bpringe reopened this Dec 30, 2020
@bpringe bpringe added bug Something isn't working jack-in enhancement and removed jack-in bug Something isn't working labels Dec 30, 2020
@PEZ
Copy link
Collaborator

PEZ commented Jan 16, 2021

I wonder what is going on here. #952 just in.

It smells race condition to me. Something in the promise-spagetti. 😄

@clyfe
Copy link

clyfe commented Jan 20, 2021

This is a todo, yes? We can fix like this?

@bpringe
Copy link
Member Author

bpringe commented Jan 20, 2021

Thanks for looking into this @clyf. That's a neat solution. I think using fs.watch or fs.exists would be more idiomatic here. Watch for it to exist with a timeout, then when it does, read it. Here's an example of using fs.watch: https://stackoverflow.com/questions/26165725/nodejs-check-file-exists-if-not-wait-till-it-exist

@PEZ
Copy link
Collaborator

PEZ commented Jan 20, 2021

This is very much like the code we have replaced. 😄

So, we used to base this on a file watcher, but now we are picking up the port from the stdout of the nrepl server when it starts. The fix will be about figuring out why the port number sometimes fails to be relayed/picked up by the connector.

Since I have never seen this happen it is a bit extra tricky for me to do the bug hunt. But for you who have seen it, if you can run Calva in dev mode and place a breakpoint right where the prompt is shown, the call stack should give us some hints.

@bpringe
Copy link
Member Author

bpringe commented Jan 20, 2021

What was the reason for moving away from the file watcher? Just wondering.

@PEZ
Copy link
Collaborator

PEZ commented Jan 20, 2021

It's just unnecessary complexity to involve the file system when we are speaking to the process directly. The reason we did it via the watcher before was that we didn't have this access then.

@clyfe
Copy link

clyfe commented Jan 21, 2021

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: remove "get port from file" code.

@clyfe clyfe mentioned this issue Jan 21, 2021
20 tasks
@clyfe
Copy link

clyfe commented Jan 21, 2021

In hindsight, does "get port from regex" cover all lein/deps/fig/shadow cases?
Do we want to have the file as fallback?

@PEZ
Copy link
Collaborator

PEZ commented Jan 21, 2021

Not even with the 20/20 hindsight. 😄 This goes back a long while. The watcher has caused me so much grief.

But you're right to be wanting to cover all bases here. The regex covers the bases you mention. Even babashka (which doesn't even write an nrepl-port file). We have yet to add support for babashka jack-in, but that is what got me to finally finish up the changes. Anyway, what we need is to make the regex configurable. We do that for some other regexes in the jack-in/connect sequences, so this should be quite easy to add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants