Skip to content

Use timeout utility instead of double-sub-shell hell #380

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

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

hhorak
Copy link
Member

@hhorak hhorak commented Aug 27, 2024

It turned out that for incremental builds for Node.js, the test always waited 10m no matter that the build finished sooner.

It's not clear why it happened, but when debugging, it seemed like some weired race-condition happened that let some sleeps hang.

Anyway, it's not clear whether there is a reason to not use timeout utility that seems to serve exactly for this reason.

@hhorak
Copy link
Member Author

hhorak commented Aug 27, 2024

@SlouchyButton Any idea why timeout could be a problem here?

@hhorak
Copy link
Member Author

hhorak commented Aug 27, 2024

[test]

@hhorak
Copy link
Member Author

hhorak commented Aug 27, 2024

[test]

@hhorak
Copy link
Member Author

hhorak commented Aug 28, 2024

[test]

Copy link

@SlouchyButton SlouchyButton left a comment

Choose a reason for hiding this comment

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

Tested on Node.js container on all supported targets and versions. All tests seem to be working.

Code is cleaner and more readable.

@SlouchyButton
Copy link

@SlouchyButton Any idea why timeout could be a problem here?

No, I don't think it could be a problem. Only thing to keep in mind is that it is part of coreutils package, so that we always have them available when running tests, but that shouldn't really be a problem. They are not part of a busybox gnu utils alternative. So it might not be available everywhere, but nor is bash strictly speaking, so I think we good.

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

LGTM. Let's spread it into containers and we will see the updates.

@phracek
Copy link
Member

phracek commented Sep 25, 2024

[test]

It turned out that for incremental builds for Node.js, the test always waited
10m no matter that the build finished sooner.

It's not clear why it happened, but when debugging, it seemed like some weired
race-condition happened that let some sleeps hang.

Anyway, it's not clear whether there is a reason to not use timeout utility
that seems to serve exactly for this reason.
@hhorak
Copy link
Member Author

hhorak commented Sep 25, 2024

[test]

@phracek
Copy link
Member

phracek commented Sep 25, 2024

Let's get merge it and distribute to s2i-nodejs-container as first.

@phracek phracek merged commit 008d6ea into master Sep 25, 2024
25 of 28 checks passed
@phracek phracek deleted the simplify-timeout branch September 25, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready for Review
Development

Successfully merging this pull request may close these issues.

3 participants