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(iteration): Pointless waiting before a known timeout #27

Merged
merged 4 commits into from
Feb 2, 2021

Conversation

sd-yip
Copy link

@sd-yip sd-yip commented Dec 19, 2020

Every time a timeout occurs, there is one second consumed for nothing.

Illustration

$ sed <wait-for 's/nc -/echo Attempt!;&/;s/sleep /echo Wait...;&/' | sh -s -- -t 3 intended.to.be.invalid:80
Attempt!
Wait...
Attempt!
Wait...
Attempt!
Wait...
Operation timed out

Since there is no attempt made after the third waiting, the outcome of such 3-seconds execution depends entirely on the first three attempts. And a known failure is always projected to one second later into the future, despite the insignificance of further waiting.

With 23bbf6d, there will be a fourth attempt:

$ sed <wait-for 's/nc -/echo Attempt!;&/;s/sleep /echo Wait...;&/' | sh -s -- -t 3 intended.to.be.invalid:80
Attempt!
Wait...
Attempt!
Wait...
Attempt!
Wait...
Attempt!
Operation timed out

It is more natural to use TIMEOUT + 1 as the maximum number of attempts.

Other changes

22d68b4 - feat(option): Restrict the timeout input to non-negative integers

$ ./wait-for -t -1 google.com:80 2>&1 | head -n1
Error: invalid timeout '-1'
$ ./wait-for -t -- google.com:80 2>&1 | head -n1
Error: invalid timeout '--'

526918b - feat(option): Support more conventional formats in the option parser

$ time ./wait-for -qt3 intended.to.be.invalid:80
Operation timed out
Command exited with non-zero status 1
real    0m 3.03s
user    0m 0.00s
sys     0m 0.01s

This is what I created this PR for in the first place.

9964904 - fix(command): Restore environment variables before calling exec

$ PORT=3000 ./wait-for -t0 google.com:80 -- sh -c 'echo "$PORT"'
3000
$ ./wait-for -t0 google.com:80 -- sh -uc 'echo "$PORT"'
sh: PORT: parameter not set

By using positional parameters, we avoid transferring the problem endlessly to other variables:

set -- "$@" -- "$TIMEOUT" "$QUIET" "$HOST" "$PORT" "$result"

@sd-yip
Copy link
Author

sd-yip commented Jan 31, 2021

@Addono Could you please help with reviewing this?

@Addono
Copy link
Member

Addono commented Jan 31, 2021

Hey! Thanks for the well documented and clean PR!

Looks good and nearly ready to be merged in.

  1. We could add some tests, e.g. I quickly drew up these, but if more test cases come to mind / you feel these can be improved them I'm all ears.
@test "support condensed option style" {
  run ./wait-for -qt1 google.com:80 -- echo 'success'

  [ "$output" = "success" ]
}

@test "timeout cannot be negative" {
  run ./wait-for -t -1 google.com:80 -- echo 'success'

  [ "$status" -ne 0 ]
  [ "$output" != "success" ]
}

@test "timeout cannot be empty" {
  run ./wait-for -t -- google.com:80 -- echo 'success'

  [ "$status" -ne 0 ]
  [ "$output" != "success" ]
}
  1. I'm a bit hesitant with merging in 9964904, your change completely makes sense and there are already several PRs addressing this issue (e.g preserve existing environment variable #4 and Prefix ENV variables to avoid clashes #13).

However, after having this script in the wild for many years, I'm sure there's someone who depends on this undocumented behavior 😦 . So there's this trade off ⚖️ between maintaining backwards compatibility and fixing bugs.

So I just added support for creating GitHub releases on master, so I was thinking about splitting this PR and first merging all commits in except 9964904. This will trigger a new release which will then be the last version with the old behavior.

Then a new PR with 9964904 can be made which will trigger a new major release with this new behavior. We're still not completely out of the woods, since I expect that many places will just pull in the latest version of the script from master, rather than a specific commit hash, but at least we would now have a release history which explains what changed between versions.

When you create the PR for 9964904, could you then have a commit message something like this:

fix(command): Restore environment variables before calling `exec`

Previously, the command would overwrite the HOST and PORT variables based on the hostname and port of the service which we would check for being available. This chance prevents these variables from being overwritten, which allows you to set them outside the command.

BREAKING CHANGE: HOST, PORT and other internally used environment variables are not overwritten anymore. If you use these, then you need to manually supply them. 

(I'm mostly interested in the BREAKING CHANGE part, since it will cause the major bump we need for this commit.)

@sd-yip
Copy link
Author

sd-yip commented Feb 1, 2021

@Addono I will create another pull request for 9964904 and have excluded it from this PR. Test cases from your suggestion are included as well. Please take a look.

@Addono Addono merged commit 12fe371 into eficode:master Feb 2, 2021
@Addono
Copy link
Member

Addono commented Feb 2, 2021

Thanks for your outstandingly well-documented PR! 🚀

@github-actions
Copy link

github-actions bot commented Feb 2, 2021

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

2 participants