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

avoid race conditions by waiting one polling interval #1697

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

gflarity
Copy link
Contributor

What does this PR do?

I've observed intermittent failures with TestShouldStartContainersInParallel. After digging into it I realized that there's a race in http WaitUntilReady. There's a brief period of time where no error is returned, but the external port hasn't been bound yet. To fix this I simply updated the logic so that one interval passes before we check the ports for the first time. This seems to do the trick and the test doesn't fail intermittently for me now.

The interval time still seems like an arbitrary sleep though. I also tried a different solution which retried until the port gets bound (or context timed out), but then you would need to either:

  1. Change multiple other tests to accommodate timeouts and get rid entire concept of "No exposed tcp ports or mapped ports - cannot wait for status" errors, as these would cease to exist.
  2. Introduce some sort of separate port binding timeout concept, which would change the API.

I think 1) probably makes the most sense to me. Waiting for http on a container with no exposed ports seems like it should timeout. But this would be a much bigger change.

Why is it important?

Intermittently failed tests make it hard to develop :)

Related issues

N/A

@gflarity gflarity requested a review from a team as a code owner September 28, 2023 18:20
@netlify
Copy link

netlify bot commented Sep 28, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit fbe9c28
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/6515ec76d5dd3b0008f514d3
😎 Deploy Preview https://deploy-preview-1697--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

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.

Thanks for taking the time to look at this issue. Race conditions are always hard to find/debug/fix, so all my gratitude for your work here.

LGTM!

@mdelapenya mdelapenya self-assigned this Sep 29, 2023
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Sep 29, 2023
@mdelapenya mdelapenya merged commit f76cdc8 into testcontainers:main Sep 29, 2023
106 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants