KEMBAR78
fix: Remove race condition in test. by tom-andersen · Pull Request #1826 · googleapis/java-firestore · GitHub
Skip to content

Conversation

@tom-andersen
Copy link
Contributor

@tom-andersen tom-andersen commented Sep 17, 2024

CI sometimes fails on listenToDocumentsWithVectors, and as best as I can tell, it is because of race condition in test.

There is a blocking call on listener that precedes incrementing semaphore that controls switch statement. Consequently, the case that deletes the document runs twice. The second time, the document has been deleted, so assertion fails on assertNotNull(docSnap) line 1456. This can happen if the executor on listener is a thread pool capable of concurrent execution, allowing next snapshot to process, before previous snapshot is done processing.

This PR makes the code more readable, and removes any chance of race condition.

I have applied this to both places where this pattern appears.

@tom-andersen tom-andersen requested a review from a team as a code owner September 17, 2024 15:48
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: firestore Issues related to the googleapis/java-firestore API. labels Sep 17, 2024
@tom-andersen tom-andersen added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2024
Copy link
Contributor

@MarkDuckworth MarkDuckworth 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 for the fix!

@tom-andersen tom-andersen merged commit 791281e into main Sep 17, 2024
24 checks passed
@tom-andersen tom-andersen deleted the tomandersen/fixRaceCondition branch September 17, 2024 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: firestore Issues related to the googleapis/java-firestore API. size: s Pull request size is small.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants