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 racy X11 forwarding test #48045

Merged
merged 1 commit into from
Oct 29, 2024
Merged

Fix racy X11 forwarding test #48045

merged 1 commit into from
Oct 29, 2024

Conversation

zmb3
Copy link
Collaborator

@zmb3 zmb3 commented Oct 28, 2024

{require,assert}.Eventually run the passed in func on a new goroutine for each attempt, so it's not safe to write to a shared variable without any synchronization.

Closes #47756

{require,assert}.Eventually run the passed in func on a new goroutine
for each attempt, so it's not safe to write to a shared variable
without any synchronization.

Closes #47756
@@ -4749,15 +4749,15 @@ func testX11Forwarding(t *testing.T, suite *integrationTestSuite) {
assert.Eventually(t, func() bool {
output, err := os.ReadFile(tmpFile.Name())
if err == nil && len(output) != 0 {
display = strings.TrimSpace(string(output))
display <- strings.TrimSpace(string(output))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think a buffer of 1 should be okay, since as soon as this write occurs the entire require.EventuallyWithT should complete, but would appreciate more 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Could make it a non-blocking channel write that just discards the value if there's something in the channel already?

Copy link
Contributor

@espadolini espadolini left a comment

Choose a reason for hiding this comment

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

The fact that the Eventually functions can overlap in execution is insane, and I'm now worried if there's other tests where we assume they don't. 😰

@zmb3
Copy link
Collaborator Author

zmb3 commented Oct 29, 2024

The fact that the Eventually functions can overlap in execution is insane, and I'm now worried if there's other tests where we assume they don't. 😰

Agreed.

@zmb3 zmb3 added the no-changelog Indicates that a PR does not require a changelog entry label Oct 29, 2024
@zmb3 zmb3 enabled auto-merge October 29, 2024 14:35
@zmb3 zmb3 added this pull request to the merge queue Oct 29, 2024
Merged via the queue into master with commit 690a0bd Oct 29, 2024
41 of 42 checks passed
@zmb3 zmb3 deleted the zmb3/x11-test-race branch October 29, 2024 14:54
@public-teleport-github-review-bot

@zmb3 See the table below for backport results.

Branch Result
branch/v15 Create PR
branch/v16 Failed
branch/v17 Create PR

zmb3 added a commit that referenced this pull request Oct 29, 2024
{require,assert}.Eventually run the passed in func on a new goroutine
for each attempt, so it's not safe to write to a shared variable
without any synchronization.

Closes #47756
github-merge-queue bot pushed a commit that referenced this pull request Oct 29, 2024
{require,assert}.Eventually run the passed in func on a new goroutine
for each attempt, so it's not safe to write to a shared variable
without any synchronization.

Closes #47756
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.

TestIntegrations/X11Forwarding flakiness
3 participants