search

hashicorp / go-retryablehttp

Retryable HTTP client in Go
Mozilla Public License 2.0
1.99k stars 251 forks source link

`defaultClient` causes a goroutine leak #214

Closed miparnisari closed 5 months ago

miparnisari commented 9 months ago

This simple test passes:

package main

import (
    "testing"

    "github.com/hashicorp/go-retryablehttp"
    "github.com/stretchr/testify/require"
)

func TestSomething(t *testing.T) {
    //defer goleak.VerifyNone(t)
    resp, err := retryablehttp.Get("https://www.example.com")
    require.Equal(t, 200, resp.StatusCode)
    require.NoError(t, err)
}

But this one fails:

package main

import (
    "testing"

    "github.com/hashicorp/go-retryablehttp"
    "github.com/stretchr/testify/require"
    "go.uber.org/goleak"
)

func TestSomething(t *testing.T) {
    defer goleak.VerifyNone(t)
    resp, err := retryablehttp.Get("https://www.example.com")
    require.Equal(t, 200, resp.StatusCode)
    require.NoError(t, err)
}

I get 

=== RUN   TestSomething
2024/01/26 00:36:28 [DEBUG] GET https://www.example.com
    main_test.go:16: found unexpected goroutines:
        [Goroutine 36 in state IO wait, with internal/poll.runtime_pollWait on top of the stack:
        internal/poll.runtime_pollWait(0x14944ce90, 0x72)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/runtime/netpoll.go:343 +0xa0
        internal/poll.(*pollDesc).wait(0x14000184000?, 0x14000248000?, 0x0)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/internal/poll/fd_poll_runtime.go:84 +0x28
        internal/poll.(*pollDesc).waitRead(...)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/internal/poll/fd_poll_runtime.go:89
        internal/poll.(*FD).Read(0x14000184000, {0x14000248000, 0x1300, 0x1300})
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/internal/poll/fd_unix.go:164 +0x200
        net.(*netFD).Read(0x14000184000, {0x14000248000?, 0x1400020e3c0?, 0x1023464f4?})
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/net/fd_posix.go:55 +0x28
        net.(*conn).Read(0x14000206000, {0x14000248000?, 0x140000df878?, 0x10234813c?})
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/net/net.go:179 +0x34
        crypto/tls.(*atLeastReader).Read(0x140002323c0, {0x14000248000?, 0x140002323c0?, 0x0?})
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/crypto/tls/conn.go:805 +0x40
        bytes.(*Buffer).ReadFrom(0x1400020a2a8, {0x1024df980, 0x140002323c0})
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/bytes/buffer.go:211 +0x90
        crypto/tls.(*Conn).readFromUntil(0x1400020a000, {0x14948ffb0?, 0x14000206000}, 0x140002480f8?)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/crypto/tls/conn.go:827 +0xd0
        crypto/tls.(*Conn).readRecordOrCCS(0x1400020a000, 0x0)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/crypto/tls/conn.go:625 +0x1e4
        crypto/tls.(*Conn).readRecord(...)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/crypto/tls/conn.go:587
        crypto/tls.(*Conn).Read(0x1400020a000, {0x14000275000, 0x1000, 0x1023a34ac?})
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/crypto/tls/conn.go:1369 +0x168
        bufio.(*Reader).Read(0x14000268e40, {0x1400025c4a0, 0x9, 0x102691eb0?})
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/bufio/bufio.go:244 +0x1b4
        io.ReadAtLeast({0x1024dedd8, 0x14000268e40}, {0x1400025c4a0, 0x9, 0x9}, 0x9)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/io/io.go:335 +0xa0
        io.ReadFull(...)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/io/io.go:354
        net/http.http2readFrameHeader({0x1400025c4a0, 0x9, 0x205740?}, {0x1024dedd8?, 0x14000268e40?})
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/net/http/h2_bundle.go:1635 +0x58
        net/http.(*http2Framer).ReadFrame(0x1400025c460)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/net/http/h2_bundle.go:1899 +0x78
        net/http.(*http2clientConnReadLoop).run(0x140000dff88)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/net/http/h2_bundle.go:9339 +0xf8
        net/http.(*http2ClientConn).readLoop(0x14000272000)
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/net/http/h2_bundle.go:9234 +0x5c
        created by net/http.(*http2Transport).newClientConn in goroutine 35
            /opt/homebrew/Cellar/go/1.21.6/libexec/src/net/http/h2_bundle.go:7906 +0xabc
        ]
--- FAIL: TestSomething (1.33s)

FAIL

It's most likely because of this: https://github.com/hashicorp/go-retryablehttp/blob/4165cf8897205a879a06b20d1ed0a2a76fbb6a17/client.go#L60

The problem with this leak is that any consumer of this package that uses the leak detector will get a failure.

I think this package shouldn't create a global variable, it should create a client when it needs one.

miparnisari commented 5 months ago

This fixes it:

func TestSomething(t *testing.T) {
    t.Cleanup(func() {
        goleak.VerifyNone(t)
    })
    c := retryablehttp.NewClient()
    resp, err := c.Get("https://www.example.com")
    require.Equal(t, 200, resp.StatusCode)
    require.NoError(t, err)

    err = resp.Body.Close()
    require.NoError(t, err)
    c.HTTPClient.CloseIdleConnections()
}