Skip to content

Fix panic for Eventually functions#808

Merged
georgelesica-wf merged 1 commit intostretchr:masterfrom
jszwec:issue-805
Nov 6, 2019
Merged

Fix panic for Eventually functions#808
georgelesica-wf merged 1 commit intostretchr:masterfrom
jszwec:issue-805

Conversation

@jszwec
Copy link
Contributor

@jszwec jszwec commented Aug 29, 2019

gmiejski
gmiejski previously approved these changes Sep 2, 2019
Copy link
Contributor

@gmiejski gmiejski left a comment

Choose a reason for hiding this comment

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

Looks fine for me, thanks for that 😄

@georgelesica-wf georgelesica-wf added this to the Next Release milestone Sep 3, 2019
@georgelesica-wf georgelesica-wf self-assigned this Sep 3, 2019
@jszwec
Copy link
Contributor Author

jszwec commented Sep 3, 2019

I believe we should also consider limiting the number of condition function calls to one at the time. It is possible to have a few of these running in parallel. The caller may not be aware that under certain circumstances the condition function should be thread safe or idempotent. However, arguably this would break the current description: periodically checking target function each tick.

Let me know what you think.

@bencampbell-wf
Copy link

I am wondering if it is worth re-implementing this using a pipeline pattern as described on the Go Blog. I figure it gives us the concurrency safety we are missing in the master implementation as well as defines some components for other concurrent matchers that may come along.

I spiked an example: https://play.golang.org/p/qJcXUfgnmgA

@georgelesica-wf
Copy link

Can we consider the above suggestion?

@jszwec
Copy link
Contributor Author

jszwec commented Sep 11, 2019

I am not against using pipeline pattern, because I use it a lot in my daily work. However, in my personal opinion, we should settle for something smaller and tailored specifically for the job.

We could achieve the same by just disabling the ticker channel like this: https://play.golang.org/p/GhfafyX9epo

@j0z3f
Copy link

j0z3f commented Sep 26, 2019

I think there might be a bug in this PR.
Isn't it technically possible (although practically very unlikely) that the select statement in

select {
case checkPassed <- condition():
default:
}

jumps to the default: branch because the result from previous invocation of condition() has not been read from checkPassed yet. In this way, the code could miss a true return value and fail.

(I came across this PR because I ran into the issue #805)

@jszwec
Copy link
Contributor Author

jszwec commented Sep 26, 2019

It's true, but without it you risk go routine leaks, these are tests, but still... We could increase the buffer, but to what end

It seems that we did not decide what the final approach should be yet. Like I mentioned in previous comments, in my opinion we should not be calling condition multiple times in parallel. In this case I think the problem that you mentioned wouldn't exist. I wrote an example above

@perhallgren
Copy link

Since you're no longer closing the checkPassed channel, I think this would also resolve #835.

@jszwec
Copy link
Contributor Author

jszwec commented Nov 6, 2019

@georgelesica-wf I updated the code with the approach that I proposed above. Tests are failing on Go tip, it's a go mod related failure unrelated to this PR.

Copy link

@georgelesica-wf georgelesica-wf left a comment

Choose a reason for hiding this comment

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

Thanks for your patience!

@georgelesica-wf georgelesica-wf merged commit f1bd092 into stretchr:master Nov 6, 2019
tadaskay added a commit to mysteriumnetwork/node that referenced this pull request Jan 31, 2020
stretchr/testify#808 is merged into master,
but not yet released as of moment of writing. Updating to latest master version.
tadaskay added a commit to mysteriumnetwork/node that referenced this pull request Jan 31, 2020
stretchr/testify#808 is merged into master,
but not yet released as of moment of writing. Updating to latest master version.
tadaskay added a commit to mysteriumnetwork/node that referenced this pull request Jan 31, 2020
stretchr/testify#808 is merged into master,
but not yet released as of moment of writing. Updating to latest master version.
@narqo narqo mentioned this pull request Feb 18, 2020
gitlab-runner-bot pushed a commit to gitlabhq/gitlab-runner that referenced this pull request Oct 2, 2020
@dolmen dolmen added pkg-assert Change related to package testify/assert assert.Eventually About assert.Eventually/EventuallyWithT labels Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

assert.Eventually About assert.Eventually/EventuallyWithT bug pkg-assert Change related to package testify/assert

Projects

None yet

Development

Successfully merging this pull request may close these issues.

race condition in Eventually assert.Eventually with low ticks cause panic: send on closed channel

7 participants