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

Get rid of nap with gopeek #8

Closed
wants to merge 5 commits into from

Conversation

cat2neat
Copy link

The below is quoted from #1 (diff) .

Alternatively, if this is purely for testing, we could have a "sleep till there's no runnable goroutines" function. There's no easy way to do it, but we'd get a list of all goroutines, and make sure that they're all blocked on something.

As I felt there are not negligible such needs especially for testing
I've developed https://github.com/cat2neat/gopeek to realise the above requirements in a generalized way.

This PR would

  • get rid of nap with gopeek
  • get the test time speed up around 3times.
  • get the test more stable in a resource constrained one like travis-ci as time.Sleep tend to be unstable

@CLAassistant
Copy link

CLAassistant commented Dec 10, 2016

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Fantastic. Will need to update glide.yaml.

Copy link
Contributor

@prashantv prashantv 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 the PR @cat2neat

I'm a little weary of an interface with a custom language for equals, condition checks etc, when the programming language has these checks. See related FAQ question:
https://golang.org/doc/faq#assertions

The simple cases are fine, but what happens when I want to wait until there's 2 goroutines waiting for a channel, and one goroutine waiting on a lock?

At the same time, I do appreciate the convenience (which is why we use testify), I just want to make sure the API allows for more flexibility if required.

@@ -5,3 +5,4 @@ testImport:
subpackages:
- assert
- package: github.com/uber-go/atomic
- package: github.com/cat2neat/gopeek
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also do glide up so that glide.lock is also updated

Copy link
Author

Choose a reason for hiding this comment

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

done.

m.Lock()
// Calculate the final time.
end := m.now.Add(d)

for len(m.timers) > 0 && m.now.Before(end) {
// Wait before popping from timers
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why we need to wait before popping timers? I would expect a wait after popping a timer.

Copy link
Author

@cat2neat cat2neat Dec 12, 2016

Choose a reason for hiding this comment

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

Without nap ( Strictly speaking the current test can fail even with nap if a gorotine executing test wake up before workers start to run. actually I've faced sometimes while testing )
there is a possibility that

t := heap.Pop(&m.timers).(*Timer)

may be invoked before one of workers running job get into the point clock.Sleep .
in this case, as there are still only timers registered by clock.AfterFunc , test will fail.

Is(gopeek.StateWaitingChannel).
// go ahead when there is >= 1 waiting channel via Mock.Sleep
// as it's evidence that ratelimit occurred
GT(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Slight inconsistency here: comment says >= 1, but code says GT(0)

Copy link
Author

Choose a reason for hiding this comment

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

done.

@cat2neat
Copy link
Author

cat2neat commented Dec 12, 2016

Thank you for the FB @prashantv .

The simple cases are fine, but what happens when I want to wait until there's 2 goroutines waiting for a channel, and one goroutine waiting on a lock?
At the same time, I do appreciate the convenience (which is why we use testify), I just want to make sure the API allows for more flexibility if required.

like this.

gopeek.NewCondition().
       CreatedBy("gopeek_test.TestGoPeek.*").
       EQ(2).
       FilterByGoes(func(gs []stack.Gorotine) bool {
           for _, g := range gs {
               // `${Condition}` means any exported fields in [stack.Gorotine](https://github.com/maruel/panicparse/blob/master/stack/stack.go) can be used to identify a specific gorotine
               if g.${Condition} && gopeek.NewState(g.State) == gopeek.StateWaitingChannel {
                   continue
               } else if gopeek.NewState(g.State) == gopeek.StateWaitingLock {
                   continue
               }
               return false
           }
           return true
       }).Wait(time.Second)

The complex cases typically can be solved using user-defined filter (FilterByGoes or FilterByGo) with stack.Gorotine .

@kriskowal kriskowal dismissed their stale review December 13, 2016 01:24

Requested change addressed. Turning over to @prashantv

@cat2neat
Copy link
Author

@kriskowal @prashantv
It turned out that this PR may cause a little bit drawback especially under less vcpu on linux.
ExampleRatelimit get more flaky (though already flaky as is)
because gopeek use runtime.Gosched with a tight loop instead of nap(time.Sleep) while waiting for getting goroutines satisfy a specified condition that means ratelimit.test get more cpu intensive
so the linux default scheduler CFS tend to be late giving back a timeslice to ratelimit.test cause time.Sleep unstable like the following.

--- PASS: ExampleRatelimit (0.09s)
PASS
ok      go.uber.org/ratelimit   1.998s
=== RUN   TestUnlimited
--- PASS: TestUnlimited (0.00s)
=== RUN   TestRateLimiter
--- PASS: TestRateLimiter (0.40s)
=== RUN   TestDelayedRateLimiter
--- PASS: TestDelayedRateLimiter (1.59s)
=== RUN   ExampleRatelimit
--- PASS: ExampleRatelimit (0.09s)
PASS
ok      go.uber.org/ratelimit   2.088s
=== RUN   TestUnlimited
--- PASS: TestUnlimited (0.00s)
=== RUN   TestRateLimiter
--- PASS: TestRateLimiter (0.22s)
=== RUN   TestDelayedRateLimiter
--- PASS: TestDelayedRateLimiter (1.45s)
=== RUN   ExampleRatelimit
--- PASS: ExampleRatelimit (0.09s)
PASS
ok      go.uber.org/ratelimit   1.785s
=== RUN   TestUnlimited
--- PASS: TestUnlimited (0.00s)
=== RUN   TestRateLimiter
--- PASS: TestRateLimiter (0.32s)
=== RUN   TestDelayedRateLimiter
--- PASS: TestDelayedRateLimiter (1.63s)
=== RUN   ExampleRatelimit
--- PASS: ExampleRatelimit (0.10s)
PASS
ok      go.uber.org/ratelimit   2.048s
=== RUN   TestUnlimited
--- PASS: TestUnlimited (0.00s)
=== RUN   TestRateLimiter
--- PASS: TestRateLimiter (0.32s)
=== RUN   TestDelayedRateLimiter
--- PASS: TestDelayedRateLimiter (1.57s)
=== RUN   ExampleRatelimit
--- PASS: ExampleRatelimit (0.09s)
PASS
ok      go.uber.org/ratelimit   1.990s
=== RUN   TestUnlimited
--- PASS: TestUnlimited (0.00s)
=== RUN   TestRateLimiter
--- PASS: TestRateLimiter (0.23s)
=== RUN   TestDelayedRateLimiter
--- PASS: TestDelayedRateLimiter (1.65s)
=== RUN   ExampleRatelimit
--- FAIL: ExampleRatelimit (0.10s)
got:
1 10ms
2 10.514905ms
3 9.485095ms
4 10ms
5 10ms
6 10ms
7 10ms
8 10ms
9 10ms
want:
1 10ms
2 10ms
3 10ms
4 10ms
5 10ms
6 10ms
7 10ms
8 10ms
9 10ms
FAIL
exit status 1
FAIL    go.uber.org/ratelimit   1.988s
Error occurred at 6th test.

Workarounds I come up with are

  1. add sufficient time.Sleep to the bottom of TestDelayedRateLimiter
    or
  2. replace ratelimit.New(100) with ratelimit.New(10) // reduce the rate
    or
  3. delete "Output:" in ExampleRatelimit

IMHO,
As ExampleRatelimit is already flaky even without this PR,
2 (reduce the rate) looks promising one to me.
Thoughts?

@rabbbit
Copy link
Contributor

rabbbit commented Dec 6, 2020

Hi,

I don't think we'll be merging this (after 3 years;)).

We've made some changes to our test runners and we no longer need to time.Sleep().

@rabbbit rabbbit closed this Dec 6, 2020
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.

None yet

5 participants