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

test: use concurrent queue to avoid race condition #8875

Merged
merged 1 commit into from
Mar 14, 2022
Merged

test: use concurrent queue to avoid race condition #8875

merged 1 commit into from
Mar 14, 2022

Conversation

mjsax
Copy link
Member

@mjsax mjsax commented Mar 10, 2022

Description

What behavior do you want to change, why, how does your patch achieve the changes?

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@mjsax mjsax requested a review from vvcephei March 10, 2022 16:59
@mjsax mjsax requested a review from a team as a code owner March 10, 2022 16:59
Copy link
Contributor

@lct45 lct45 left a comment

Choose a reason for hiding this comment

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

LGTM @mjsax, thanks for the quick fix

Copy link
Member

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Thanks, @mjsax !

It seems worthwhile to point out that this was in response to a test failure:

[2022-03-10T05:51:51.892Z] [ERROR] Tests run: 8, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 203.505 s <<< FAILURE! - in io.confluent.ksql.integration.SecureIntegrationTest
[2022-03-10T05:51:51.892Z] [ERROR] io.confluent.ksql.integration.SecureIntegrationTest.shouldClassifyGroupAuthorizationExceptionAsUserError  Time elapsed: 0.511 s  <<< ERROR!
[2022-03-10T05:51:51.892Z] java.lang.ArrayIndexOutOfBoundsException
[2022-03-10T05:51:51.892Z] 	at io.confluent.ksql.integration.SecureIntegrationTest.lambda$assertQueryFailsWithUserError$5(SecureIntegrationTest.java:517)
[2022-03-10T05:51:51.892Z] 	at io.confluent.ksql.integration.SecureIntegrationTest.assertQueryFailsWithUserError(SecureIntegrationTest.java:515)
[2022-03-10T05:51:51.892Z] 	at io.confluent.ksql.integration.SecureIntegrationTest.shouldClassifyGroupAuthorizationExceptionAsUserError(SecureIntegrationTest.java:362)

While the stack trace is incomplete, digging through the Guava code indicates that the ArrayIndexOutOfBounds exception was probably thrown from the EvictingQueue due to a multithreaded race condition in toArray. Wrapping it should prevent any such bugs.

I'm not sure whether it matters, but it seems like this PR should be a fix instead of a test. I think the test was revealing a real bug in production code.

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.

3 participants