rpc: avoid unnecessary RST_STREAM, PING frames sent by client

[michaelkaplan13]

Context from Cloudflare blog: https://blog.cloudflare.com/go-and-enhance-your-calm/#reading-bodies-in-go-can-be-unintuitive

We were able to reproduce the same issue discussed by Cloudflare in their recent blog post above using the ethclient.

Running the below script using the latest go-ethereum against RPC endpoints with HTTP2 defenses similar to Cloudflare's (i.e. go run main.go --url https://api.avax-test.network/ext/bc/C/rpc) results in a number of GOAWAY errors due the client sending many unnecessary PING and RST_STREAM frames.

Running the same script with the fix in this PR results shows that those GOAWAY errors to go away. 🙂

package main

import (
	"context"
	"flag"
	"fmt"
	"log"
	"os"
	"strings"
	"sync"
	"sync/atomic"
	"time"

	"github.com/ethereum/go-ethereum/ethclient"
)

func main() {
	const numThreads = 100
	const reqsPerThread = 100

	// Command-line flags
	targetURL := flag.String("url", "", "Target RPC URL to test")
	flag.Usage = func() {
		fmt.Fprintf(os.Stderr, "Usage: %s [options]\n", os.Args[0])
		fmt.Fprintf(os.Stderr, "\nLoad testing tool for RPC endpoints\n\n")
		fmt.Fprintf(os.Stderr, "Options:\n")
		flag.PrintDefaults()
	}
	flag.Parse()

	fmt.Printf("Starting load test with %d threads, %d requests per thread\n", numThreads, reqsPerThread)
	fmt.Printf("Target URL: %s\n", *targetURL)
	fmt.Printf("Total requests: %d\n", numThreads*reqsPerThread)

	var totalErrors int64
	var goawayErrors int64

	var wg sync.WaitGroup
	startTime := time.Now()

	for i := 0; i < numThreads; i++ {
		wg.Add(1)
		go func(threadID int) {
			defer wg.Done()
			runThread(threadID, *targetURL, reqsPerThread, &totalErrors, &goawayErrors)
		}(i)
	}

	wg.Wait()

	duration := time.Since(startTime)
	totalRequests := numThreads * reqsPerThread
	fmt.Printf("Completed %d requests in %v (%.2f req/sec)\n",
		totalRequests, duration, float64(totalRequests)/duration.Seconds())

	fmt.Printf("Total errors: %d\n", atomic.LoadInt64(&totalErrors))
	fmt.Printf("GOAWAY errors: %d\n", atomic.LoadInt64(&goawayErrors))
}

func runThread(threadID int, targetURL string, reqsPerThread int, totalErrors *int64, goawayErrors *int64) {
	client, err := ethclient.Dial(targetURL)
	if err != nil {
		log.Printf("thread %d failed to connect to Ethereum client: %v", threadID, err)
		atomic.AddInt64(totalErrors, int64(reqsPerThread))
		return
	}
	defer client.Close()

	ctx := context.Background()

	for i := 0; i < reqsPerThread; i++ {
		_, err := client.BlockByNumber(ctx, nil) // nil means latest block
		if err != nil {
			atomic.AddInt64(totalErrors, 1)

			errorStr := err.Error()
			if strings.Contains(strings.ToUpper(errorStr), "GOAWAY") {
				atomic.AddInt64(goawayErrors, 1)
			}

			log.Printf("thread %d request %d errored: %v", threadID, i, err)
		}
	}
}

[fjl]

Thanks for the context. I like the change, but I think we can handle this just within package rpc. You tried to fix all occurences of body.Close() across the code, but I think it's not necessary. The main part where this matters is in the RPC client. Let's do a targeted fix there.

[michaelkaplan13]

Thanks for the context. I like the change, but I think we can handle this just within package rpc. You tried to fix all occurences of body.Close() across the code, but I think it's not necessary. The main part where this matters is in the RPC client. Let's do a targeted fix there.

Sure thing. I've limited the change to just the rpc package now.

[michaelkaplan13]

@joshua-kim also created an issue for this to be fixed upstream here: golang/go#76239

Until it is addressed there though, I think this would be a helpful workaround to provide for anyone using ethclient as a dependency.

[fjl]

Thanks for the patch!