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: timeout and shutdown #121

Merged
merged 2 commits into from
Apr 8, 2024
Merged

Conversation

stevenh
Copy link
Contributor

@stevenh stevenh commented Mar 13, 2024

Fix waitForPruneCondition timeout handling which was not resetting the connection timeout when new connections came in resulting in the reaper shutting down incorrectly.

Don't log EOF errors.

Add buffer to connection channels so we don't block the accepting goroutine.

Fixes testcontainers/testcontainers-go#2348

Fix waitForPruneCondition timeout handling which was not resetting the
connection timeout when new connections came in resulting in the reaper
shutting down incorrectly.

Don't log EOF errors.

Add buffer to connection channels so we don't block the accepting
goroutine.

Fixes testcontainers/testcontainers-go#2348
@stevenh
Copy link
Contributor Author

stevenh commented Apr 8, 2024

@mdelapenya any chance you can help get this one across the line, as its causing random failures in testcontainers-go?

@mdelapenya mdelapenya self-assigned this Apr 8, 2024
@mdelapenya mdelapenya added the bug An issue with the project label Apr 8, 2024
Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

LGTM, thanks and sorry for the delay in the review. I've been focused in other tasks regarding testcontainers-go.

Thank you for your time!

@mdelapenya mdelapenya merged commit 103d99f into testcontainers:main Apr 8, 2024
7 checks passed
@stevenh stevenh deleted the fix/timeout branch April 8, 2024 11:29
@stevenh
Copy link
Contributor Author

stevenh commented Apr 8, 2024

LGTM, thanks and sorry for the delay in the review. I've been focused in other tasks regarding testcontainers-go.

Thank you for your time!

Thanks for that, most appreciated!

To get this one fully put to bed, what's the release process, as we'll need to a PR against testcontainers-go to bump it to the new version to get this fix?

@mdelapenya
Copy link
Member

Exactly, I'm planning to create a release of Ryuk soon this week, the 0.7.1

@stevenh
Copy link
Contributor Author

stevenh commented May 3, 2024

Just checking in on the release @mdelapenya to make sure it doesn't slip though 😃

@stevenh
Copy link
Contributor Author

stevenh commented Jul 12, 2024

Hey @mdelapenya just checking in on this, as we're seeing constant test failures on very basic setups due to this, so would really appreciate a new release here and then one for testcontainers-go with the updated image.

If there's anything I can do assist let me know.

@emetsger
Copy link

We are hitting this bug as well, and I was planning on fixing it this weekend, but happy to see @stevenh beat me to it.

For posterity:

  • Rancher 1.12.2 does not seem affected
  • Rancher 1.13.1 and the latest 1.14 are affected

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: TestGenericReusableContainerInSubprocess test is flaky
3 participants