Repository: google/certificate-transparency-go (go)

Issue:
TestRateLimiterGoroutines is flaky
It looks like there was an attempt to de-flake this last year, but it's not been 100% successful: 2174cdab8364709be8f100efd4d76b3bd625ea0a
```
running go test
ok  	github.com/google/certificate-transparency-go	8.096s	coverage: 44.8% of statements
ok  	github.com/google/certificate-transparency-go/asn1	1.105s	coverage: 85.2% of statements
ok  	github.com/google/certificate-transparency-go/client	23.788s	coverage: 77.1% of statements
?   	github.com/google/certificate-transparency-go/client/ctclient	[no test files]
ok  	github.com/google/certificate-transparency-go/fixchain	2.399s	coverage: 83.3% of statements
?   	github.com/google/certificate-transparency-go/fixchain/main	[no test files]
--- FAIL: TestRateLimiterGoroutines (13.88s)
	limiter_test.go:34: #4: Too many operations per second. Expected ~1000, got 1238
FAIL
coverage: 100.0% of statements
FAIL	github.com/google/certificate-transparency-go/fixchain/ratelimiter	28.332s
```

Pull request description:
Remove flaky ratelimiter
<!---
Describe your changes in detail here.
If this fixes an issue, please write "Fixes #123", substituting the issue number.
-->

Fixes #25 

### Checklist

<!---
Go over all the following points, and put an `x` in all the boxes that apply.
Feel free to not tick any boxes that don't apply to this PR (e.g. refactoring may not need a CHANGELOG update).
If you're unsure about any of these, don't hesitate to ask. We're here to help!
-->

- [ ] I have updated the [CHANGELOG](CHANGELOG.md).
  - Adjust the draft version number according to [semantic versioning](https://semver.org/) rules.
- [ ] I have updated [documentation](docs/) accordingly.


Patch:
diff --git a/fixchain/chainfix/chainfix.go b/fixchain/chainfix/chainfix.go
index 0b66c3ec..37948696 100644
--- a/fixchain/chainfix/chainfix.go
+++ b/fixchain/chainfix/chainfix.go
@@ -28,9 +28,9 @@ import (
 
 	"github.com/google/certificate-transparency-go/client"
 	"github.com/google/certificate-transparency-go/fixchain"
-	"github.com/google/certificate-transparency-go/fixchain/ratelimiter"
 	"github.com/google/certificate-transparency-go/jsonclient"
 	"github.com/google/certificate-transparency-go/x509"
+	"golang.org/x/time/rate"
 )
 
 // Assumes chains to be stores in a file in JSON encoded with the certificates
@@ -106,7 +106,7 @@ func main() {
 	// As-is, this will log errors as strings.
 	go logStringErrors(&wg, errors, errDir)
 
-	limiter := ratelimiter.NewLimiter(1000)
+	limiter := rate.NewLimiter(rate.Limit(1000), 1)
 	c := &http.Client{}
 	logClient, err := client.New(logURL, c, jsonclient.Options{UserAgent: "ct-go-fixchain/1.0"})
 	if err != nil {
diff --git a/fixchain/logger.go b/fixchain/logger.go
index c017cd25..dc77cf86 100644
--- a/fixchain/logger.go
+++ b/fixchain/logger.go
@@ -30,7 +30,7 @@ import (
 // Limiter is an interface to allow different rate limiters to be used with the
 // Logger.
 type Limiter interface {
-	Wait()
+	Wait(context.Context) error
 }
 
 // Logger contains methods to asynchronously log certificate chains to a
@@ -163,7 +163,7 @@ func (l *Logger) postChain(p *toPost) {
 		derChain = append(derChain, ct.ASN1Cert{Data: cert.Raw})
 	}
 
-	l.limiter.Wait()
+	l.limiter.Wait(l.ctx)
 	atomic.AddUint32(&l.posted, 1)
 	_, err := l.client.AddChain(l.ctx, derChain)
 	if err != nil {
diff --git a/fixchain/ratelimiter/limiter.go b/fixchain/ratelimiter/limiter.go
deleted file mode 100644
index 2477920e..00000000
--- a/fixchain/ratelimiter/limiter.go
+++ /dev/null
@@ -1,40 +0,0 @@
-// Copyright 2016 Google LLC. All Rights Reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-// Package ratelimiter provides an exceedingly simple rate limiter.
-package ratelimiter
-
-import (
-	"context"
-
-	"golang.org/x/time/rate"
-)
-
-// Limiter is a simple rate limiter.
-type Limiter struct {
-	ctx    context.Context
-	bucket *rate.Limiter
-}
-
-// Wait blocks for the amount of time required by the Limiter so as to not
-// exceed its rate.
-func (l *Limiter) Wait() {
-	l.bucket.Wait(l.ctx)
-}
-
-// NewLimiter creates a new Limiter with a rate of limit per second.
-func NewLimiter(limit int) *Limiter {
-	return &Limiter{ctx: context.Background(),
-		bucket: rate.NewLimiter(rate.Limit(limit), 1)}
-}
diff --git a/trillian/integration/ct_hammer/main.go b/trillian/integration/ct_hammer/main.go
index 71cfccc0..4db2abde 100644
--- a/trillian/integration/ct_hammer/main.go
+++ b/trillian/integration/ct_hammer/main.go
@@ -32,7 +32,6 @@ import (
 	"time"
 
 	"github.com/google/certificate-transparency-go/client"
-	"github.com/google/certificate-transparency-go/fixchain/ratelimiter"
 	"github.com/google/certificate-transparency-go/jsonclient"
 	"github.com/google/certificate-transparency-go/loglist3"
 	"github.com/google/certificate-transparency-go/trillian/ctfe"
@@ -42,6 +41,7 @@ import (
 	"github.com/google/trillian/monitoring"
 	"github.com/google/trillian/monitoring/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
+	"golang.org/x/time/rate"
 	"k8s.io/klog/v2"
 )
 
@@ -91,11 +91,11 @@ var (
 	strictSTHConsistencySize = flag.Bool("strict_sth_consistency_size", true, "If set to true, hammer will use only tree sizes from STHs it's seen for consistency proofs, otherwise it'll choose a random size for the smaller tree")
 )
 
-func newLimiter(rate int) integration.Limiter {
-	if rate <= 0 {
+func newLimiter(limit int) integration.Limiter {
+	if limit <= 0 {
 		return nil
 	}
-	return ratelimiter.NewLimiter(rate)
+	return rate.NewLimiter(rate.Limit(limit), 1)
 }
 
 // copierGeneratorFactory returns a function that creates per-Log ChainGenerator instances
diff --git a/trillian/integration/hammer.go b/trillian/integration/hammer.go
index 4aa0b4b7..14bd1c90 100644
--- a/trillian/integration/hammer.go
+++ b/trillian/integration/hammer.go
@@ -100,12 +100,13 @@ const (
 // Limiter is an interface to allow different rate limiters to be used with the
 // hammer.
 type Limiter interface {
-	Wait()
+	Wait(context.Context) error
 }
 
 type unLimited struct{}
 
-func (u unLimited) Wait() {
+func (u unLimited) Wait(ctx context.Context) error {
+	return nil
 }
 
 // HammerConfig provides configuration for a stress/load test.
@@ -975,7 +976,7 @@ func (s *hammerState) String() string {
 }
 
 func (s *hammerState) performOp(ctx context.Context, ep ctfe.EntrypointName) (int, error) {
-	s.cfg.Limiter.Wait()
+	s.cfg.Limiter.Wait(ctx)
 
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -1013,7 +1014,7 @@ func (s *hammerState) performOp(ctx context.Context, ep ctfe.EntrypointName) (in
 }
 
 func (s *hammerState) performInvalidOp(ctx context.Context, ep ctfe.EntrypointName) error {
-	s.cfg.Limiter.Wait()
+	s.cfg.Limiter.Wait(ctx)
 	switch ep {
 	case ctfe.AddChainName:
 		return s.addChainInvalid(ctx)


Test patch:
diff --git a/fixchain/functions_test.go b/fixchain/functions_test.go
index bc38a3fb..cc9a0e94 100644
--- a/fixchain/functions_test.go
+++ b/fixchain/functions_test.go
@@ -16,6 +16,7 @@ package fixchain
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
 	"fmt"
 	"log"
@@ -29,7 +30,8 @@ import (
 
 type nilLimiter struct{}
 
-func (l *nilLimiter) Wait() {
+func (l *nilLimiter) Wait(ctx context.Context) error {
+	return nil
 }
 
 func newNilLimiter() *nilLimiter {
diff --git a/fixchain/ratelimiter/limiter_test.go b/fixchain/ratelimiter/limiter_test.go
deleted file mode 100644
index 5098f7a2..00000000
--- a/fixchain/ratelimiter/limiter_test.go
+++ /dev/null
@@ -1,86 +0,0 @@
-// Copyright 2016 Google LLC. All Rights Reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package ratelimiter
-
-import (
-	"fmt"
-	"sync"
-	"testing"
-	"time"
-
-	"k8s.io/klog/v2"
-)
-
-var testlimits = []int{1, 10, 50, 100, 1000}
-
-func TestRateLimiterSingleThreaded(t *testing.T) {
-	for i, limit := range testlimits {
-		t.Run(fmt.Sprintf("%d ops/s", limit), func(t *testing.T) {
-			i, limit := i, limit
-			t.Parallel()
-			l := NewLimiter(limit)
-
-			numOps := 3 * limit
-			start := time.Now()
-			// Need to call the limiter one extra time to ensure that the throughput
-			// calculation is correct (because e.g. at 1 qps you can do 3 calls in
-			// 2+epsilon seconds)
-			for i := 0; i < numOps+1; i++ {
-				l.Wait()
-			}
-			ds := float64(time.Since(start)) / float64(time.Second)
-			qps := float64(numOps) / ds
-			// The rate limit is temporarily updated to 2 due to the flakiness of this test. The original rate limit is 1.01.
-			// https://github.com/google/certificate-transparency-go/issues/951
-			if qps > float64(limit)*2 {
-				t.Errorf("#%d: Too many operations per second. Expected ~%d, got %f", i, limit, qps)
-			}
-			klog.Infof("#%d: Expected ~%d, got %f", i, limit, qps)
-		})
-	}
-}
-
-func TestRateLimiterGoroutines(t *testing.T) {
-	for i, limit := range testlimits {
-		t.Run(fmt.Sprintf("%d ops/s", limit), func(t *testing.T) {
-			i, limit := i, limit
-			t.Parallel()
-			l := NewLimiter(limit)
-
-			numOps := 3 * limit
-			var wg sync.WaitGroup
-			start := time.Now()
-			// Need to call the limiter one extra time to ensure that the throughput
-			// calculation is correct (because e.g. at 1 qps you can do 3 calls in
-			// 2+epsilon seconds)
-			for i := 0; i < numOps+1; i++ {
-				wg.Add(1)
-				go func() {
-					l.Wait()
-					wg.Done()
-				}()
-			}
-			wg.Wait()
-			ds := float64(time.Since(start)) / float64(time.Second)
-			qps := float64(numOps) / ds
-			// The rate limit is temporarily updated to 2 due to the flakiness of this test. The original rate limit is 1.01.
-			// https://github.com/google/certificate-transparency-go/issues/951
-			if qps > float64(limit)*2 {
-				t.Errorf("#%d: Too many operations per second. Expected ~%d, got %f", i, limit, qps)
-			}
-			klog.Infof("#%d: Expected ~%d, got %f", i, limit, qps)
-		})
-	}
-}