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 data race in AssertLogContains and functional race in TestDocChangedLogging #6517

Merged
merged 2 commits into from
Oct 11, 2023

Conversation

bbrks
Copy link
Member

@bbrks bbrks commented Oct 11, 2023

  • Ensures TestDocChangedLogging waits for a marker doc to be recieved, as we can't reliably wait for the no-op of non-metadata doc write to happen to assert on logs.
  • Forces console log buffer flush before checking contents, instead of retry loop.
  • Avoids multiple buffer reads during log writes
    • Prevents race if the log write causes buffer to grow during read inside retry loop
    • Prevents functional issues if buffer read was in the middle of the log line causing failed assertions (log lines split over multiple buffer reads)

Reliably tested before/after fix with:

$ go test -run='^TestDocChangedLogging$' -race -count=1000 -v ./rest/changestest

Integration Tests

  • n/a

…ore checking contents - avoids multiple buffer reads during log writes
@bbrks bbrks mentioned this pull request Oct 11, 2023
1 task
@bbrks
Copy link
Member Author

bbrks commented Oct 11, 2023

... this made the test fail way more reliably without a data race 🤔

There's something else going on here that I'd need to investigate

@bbrks bbrks closed this Oct 11, 2023
@bbrks bbrks reopened this Oct 11, 2023
@bbrks bbrks changed the title Fix data race in AssertLogContains Fix data race in AssertLogContains and functional race in TestDocChangedLogging Oct 11, 2023
@bbrks bbrks requested a review from torcolvin October 11, 2023 13:12
@torcolvin torcolvin merged commit a142ec8 into master Oct 11, 2023
26 checks passed
@torcolvin torcolvin deleted the fix_race_in_AssertLogContains branch October 11, 2023 14:43
torcolvin pushed a commit that referenced this pull request Oct 31, 2023
…gedLogging (#6517)

* Fix race in AssertLogContains by forcing console log buffer flush before checking contents - avoids multiple buffer reads during log writes
* Fix TestDocChangedLogging test race
bbrks added a commit that referenced this pull request Mar 28, 2024
…gedLogging (#6517)

* Fix race in AssertLogContains by forcing console log buffer flush before checking contents - avoids multiple buffer reads during log writes
* Fix TestDocChangedLogging test race
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants