Fix panic for Eventually functions

[jszwec]

Fixes #805
Fixes #835

/cc @gmiejski @georgelesica-wf

[gmiejski]

Looks fine for me, thanks for that 😄

[jszwec]

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]

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]

Can we consider the above suggestion?

[jszwec]

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]

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]

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]

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

[jszwec]

@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.

[georgelesica-wf]

Thanks for your patience!