Issue 1673: modifications in safe_sleep function

[ruvceskistefan]

Problem was the implementation of sleep in the case when both ping and sleep are missing in safe_sleep function.

Related issue: #1673

After this change, the problem described in the related issue has been fixed.

[mathieu-aubin]

the PR is somewhat okay... i just want to specify something:

the reason i used the trap method and the DEBUG signal is that when both are combined, the trap is executed everytime the/a function starts - which was great for making certain i had the most accurate times without adding extra commands time

That being said...

You should instead reset the SECONDS=0 variable before you enter the while loop. This way, it is ran only when needed and no trap will run exessively - maybe read on that?

As i said, also, having the rest of the code in there becomes rather useless since the SECONDS (with bash, only) is internal and guaranteed. Checking for other stuff is a waste and so is using anything else, specially when uncertain of their existence.

function safe_sleep() { SECONDS=0; while [[ $SECONDS != 5 ]]; do :; done }

also... it would be a good habbit to comment unusual code like trap and SECONDS for future references by others. Its certainly obscure for MANY and would benefit from precisions.

Enjoy

[ruvceskistefan]

Thanks for the reply, I replaced the trap method with resetting SECONDS=0 before the while loop. As for other if conditions and unnecessary code, let them stay for now just in case, because SECONDS isn't POSIX-compliant, so POSIX-features-only shells (dash on Ubuntu) may have some problems.

[fhammerl]

@ruvceskistefan Regarding launching the runner using, say, dash: Admittedly I'm not super familiar with the topic, but given that we're executing run-helper.sh (see run.sh for details), not sourcing it, wouldn't its first line #!/bin/bash, mean that run-helper would be interpreted by bash anyway?

Meaning, don't we already have a dependency on bash here? That would suggest to me $SECONDS is fine.

[ruvceskistefan]

@fhammerl I think you're right, then I'll get the unnecessary code out of safe_sleep function.
Thank you both @fhammerl and @mathieu-aubin for your suggestions and contributions!

[fhammerl]

Approved!

@ruvceskistefan if you're up for some follow-up, it may be worth looking into other usages of sleep where we don't have a safe fallback.

[ruvceskistefan]

Thanks @fhammerl! Of course, I'll move on that after I solve the priority issues.

[ruvceskistefan]

This does not have to be merged now, since the PR#1707 has already been merged. That PR is an upgrade to this (in accordance with the @fhammerl's feature request from the comments), so there is no need for this PR to be merged. Therefore, this PR can be closed.