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

assert.EventuallyWithT: CollectT is not protected against concurrent use #1418

Open
dolmen opened this issue Jul 11, 2023 · 7 comments
Open
Labels
bug concurrency pkg-assert Change related to package testify/assert

Comments

@dolmen
Copy link
Collaborator

dolmen commented Jul 11, 2023

In assert.EventuallyWithT, if the tick is small, the calls to the condition may overlap (run simultaneously). However they share an access to an assert.CollectT instance which is not protected for concurrent use. Race condition may occur.

@dolmen dolmen added bug pkg-assert Change related to package testify/assert concurrency labels Jul 11, 2023
@dolmen
Copy link
Collaborator Author

dolmen commented Jul 11, 2023

Cc: @brackendawson

@dolmen dolmen changed the title assert.EventuallyWithT: race condition because assert.EventuallyWithT: CollectT is not protected against concurrent use Jul 11, 2023
@dolmen
Copy link
Collaborator Author

dolmen commented Jul 11, 2023

I wonder if we should allow to run the condition function in parallel (and skip ticks) or if instead we should fix error collection to have separate instance for each call.

I have no idea how EventuallyWithT is used in the wild.

@dolmen
Copy link
Collaborator Author

dolmen commented Jul 12, 2023

There is already a pending MR that aims to fix this: #1395

@dolmen
Copy link
Collaborator Author

dolmen commented Jul 12, 2023

Duplicate of #1391.

@dolmen
Copy link
Collaborator Author

dolmen commented Jul 25, 2023

#1396 is also related, but not a duplicate.

@cszczepaniak
Copy link

cszczepaniak commented Jul 30, 2023

#1438 proposes a solution to this that does not require adding any lock objects (i.e. assert.CollectT doesn't actually have to be threadsafe to solve this problem)

Edit: actually, it probably still does require a mutex. #1438 demonstrates the bug that occurs because of this issue, though.

@dolmen
Copy link
Collaborator Author

dolmen commented Jul 30, 2023

#1395 is a way to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug concurrency pkg-assert Change related to package testify/assert
Projects
None yet
Development

No branches or pull requests

2 participants