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/robustify wait_for_port function #1695

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

ppenguin
Copy link
Contributor

This happened (posted to discord yesterday):

I'm now since a few months using devenv again, and get a strange error for `wait_for_port 34000` => `bash: line 1: /dev/tcp/localhost/34000: Connection refused`??? (Executed within `enterTest`) This confuses me, when I contributed to `devenv`'s `postgres` service a few months ago `wait_for_port` trivially worked?

This seems to be a bug (see e.g. https://unix.stackexchange.com/questions/777846/stdin-redirection-from-dev-tcp-localhost-port for a bit more insight), or at least the method used for port checking is not robust and not portable; this PR aims to fix this.

@sandydoo
Copy link
Member

sandydoo commented Feb 1, 2025

The change looks good to me, but could you describe the issue you're trying to fix in a bit more detail? I've read through the link and, while interesting, I don't see how it applies here + the portability issues also don't really apply.

Does wait_for_port completely fail in certain scenarios? Or were you surprised to see a connection error before it eventually succeeded (which is normal)?

@ppenguin
Copy link
Contributor Author

ppenguin commented Feb 1, 2025

Does wait_for_port completely fail in certain scenarios? Or were you surprised to see a connection error before it eventually succeeded (which is normal)?

It completely fails in the way the link describes, and in my case it's not even a special scenario: I'm on a normal NixOS installation in a normal shell (albeit zsh). So I can't really explain why it has worked for me before (about 6 months ago when working with the postgres service within devenv), but now it most certainly doesn't: it consistently gives the error described (i.e. /dev/tcp/ doesn't even exist).

So rather than checking how to solve the fact that I don't have /dev/tcp even on a "normal Linux system", I think the time is better spent to make this function portable in the first place. (One could argue that a working /dev/tcp could almost be considered a "large" edge case, i.e. it relies on a "nice to have" feature of bash, which I think is a good thing to avoid).

@sandydoo
Copy link
Member

sandydoo commented Feb 3, 2025

It completely fails in the way the link describes

What I really need is a clear description of the issue you experienced. The link is not enough, because it talks about a different scenario, i.e. reading from the virtual file before anything is bound to the port. We expect the initial writes to fail with an error and netcat will fail in exactly the same way.

So from what I gather, you saw multiple "connection refused" error messages, after which wait_for_port timed out, failing the test. Is that correct?

@ppenguin
Copy link
Contributor Author

ppenguin commented Feb 3, 2025

Sorry for being a bit sloppy on the error.
Since I was not trying to write to the port (as in the link), it failed similarly, but not identical, i.e. I believe I got file not found, but in any case it was an error causing wait_for_port to terminate with a bash error instead of actually waiting for the timeout.

I just tried to reproduce it however, and couldn't, I have no idea why. (Now I simply get Connection refused every 0.5s which is, I suppose, expected for the current implementation, even though annoying).

Anyway, the proposed solution did solve the file not found issue, and additionally improves the UX (clean timeout without additional error spamming). It would likely also improve portability (e.g. BSD is rumoured to behave differently?), though I haven't tested on e.g. darwin and such.

@sandydoo sandydoo merged commit ebaa744 into cachix:main Feb 8, 2025
260 of 275 checks passed
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