Skip to content

Commit 53a2aae

Browse files
authored
Fix missing Close when error occurs and abused connection pool (#35658) (#35670)
Backport #35658
1 parent 5ae9bb4 commit 53a2aae

File tree

7 files changed

+117
-165
lines changed

7 files changed

+117
-165
lines changed

go.mod

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ require (
3535
github.com/bohde/codel v0.2.0
3636
github.com/buildkite/terminal-to-html/v3 v3.16.8
3737
github.com/caddyserver/certmagic v0.24.0
38-
github.com/charmbracelet/git-lfs-transfer v0.2.0
38+
github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20251013092601-6327009efd21
3939
github.com/chi-middleware/proxy v1.1.1
4040
github.com/dimiro1/reply v0.0.0-20200315094148-d0136a4c9e21
4141
github.com/djherbis/buffer v1.2.0
@@ -56,7 +56,7 @@ require (
5656
github.com/go-co-op/gocron v1.37.0
5757
github.com/go-enry/go-enry/v2 v2.9.2
5858
github.com/go-git/go-billy/v5 v5.6.2
59-
github.com/go-git/go-git/v5 v5.16.2
59+
github.com/go-git/go-git/v5 v5.16.3
6060
github.com/go-ldap/ldap/v3 v3.4.11
6161
github.com/go-redsync/redsync/v4 v4.13.0
6262
github.com/go-sql-driver/mysql v1.9.3
@@ -121,7 +121,7 @@ require (
121121
golang.org/x/net v0.44.0
122122
golang.org/x/oauth2 v0.30.0
123123
golang.org/x/sync v0.17.0
124-
golang.org/x/sys v0.36.0
124+
golang.org/x/sys v0.37.0
125125
golang.org/x/text v0.30.0
126126
google.golang.org/grpc v1.75.0
127127
google.golang.org/protobuf v1.36.8
@@ -298,9 +298,6 @@ replace github.com/hashicorp/go-version => github.com/6543/go-version v1.3.1
298298

299299
replace github.com/nektos/act => gitea.com/gitea/act v0.261.7-0.20251003180512-ac6e4b751763
300300

301-
// TODO: the only difference is in `PutObject`: the fork doesn't use `NewVerifyingReader(r, sha256.New(), oid, expectedSize)`, need to figure out why
302-
replace github.com/charmbracelet/git-lfs-transfer => gitea.com/gitea/git-lfs-transfer v0.2.0
303-
304301
replace git.sr.ht/~mariusor/go-xsd-duration => gitea.com/gitea/go-xsd-duration v0.0.0-20220703122237-02e73435a078
305302

306303
exclude github.com/gofrs/uuid v3.2.0+incompatible

go.sum

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,6 @@ filippo.io/edwards25519 v1.1.0 h1:FNf4tywRC1HmFuKW5xopWpigGjJKiJSV0Cqo0cJWDaA=
3333
filippo.io/edwards25519 v1.1.0/go.mod h1:BxyFTGdWcka3PhytdK4V28tE5sGfRvvvRV7EaN4VDT4=
3434
gitea.com/gitea/act v0.261.7-0.20251003180512-ac6e4b751763 h1:ohdxegvslDEllZmRNDqpKun6L4Oq81jNdEDtGgHEV2c=
3535
gitea.com/gitea/act v0.261.7-0.20251003180512-ac6e4b751763/go.mod h1:Pg5C9kQY1CEA3QjthjhlrqOC/QOT5NyWNjOjRHw23Ok=
36-
gitea.com/gitea/git-lfs-transfer v0.2.0 h1:baHaNoBSRaeq/xKayEXwiDQtlIjps4Ac/Ll4KqLMB40=
37-
gitea.com/gitea/git-lfs-transfer v0.2.0/go.mod h1:UrXUCm3xLQkq15fu7qlXHUMlrhdlXHoi13KH2Dfiits=
3836
gitea.com/gitea/go-xsd-duration v0.0.0-20220703122237-02e73435a078 h1:BAFmdZpRW7zMQZQDClaCWobRj9uL1MR3MzpCVJvc5s4=
3937
gitea.com/gitea/go-xsd-duration v0.0.0-20220703122237-02e73435a078/go.mod h1:g/V2Hjas6Z1UHUp4yIx6bATpNzJ7DYtD0FG3+xARWxs=
4038
gitea.com/go-chi/binding v0.0.0-20240430071103-39a851e106ed h1:EZZBtilMLSZNWtHHcgq2mt6NSGhJSZBuduAlinMEmso=
@@ -219,6 +217,8 @@ github.com/cention-sany/utf7 v0.0.0-20170124080048-26cad61bd60a h1:MISbI8sU/PSK/
219217
github.com/cention-sany/utf7 v0.0.0-20170124080048-26cad61bd60a/go.mod h1:2GxOXOlEPAMFPfp014mK1SWq8G8BN8o7/dfYqJrVGn8=
220218
github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
221219
github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
220+
github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20251013092601-6327009efd21 h1:2d64+4Jek9vjYwhY93AjbleiVH+AeWvPwPmDi1mfKFQ=
221+
github.com/charmbracelet/git-lfs-transfer v0.1.1-0.20251013092601-6327009efd21/go.mod h1:fNlYtCHWTRC8MofQERZkVUNUWaOvZeTBqHn/amSbKZI=
222222
github.com/chi-middleware/proxy v1.1.1 h1:4HaXUp8o2+bhHr1OhVy+VjN0+L7/07JDcn6v7YrTjrQ=
223223
github.com/chi-middleware/proxy v1.1.1/go.mod h1:jQwMEJct2tz9VmtCELxvnXoMfa+SOdikvbVJVHv/M+0=
224224
github.com/chromedp/cdproto v0.0.0-20230802225258-3cf4e6d46a89/go.mod h1:GKljq0VrfU4D5yc+2qA6OVr8pmO/MBbPEWqWQ/oqGEs=
@@ -339,8 +339,8 @@ github.com/go-git/go-billy/v5 v5.6.2 h1:6Q86EsPXMa7c3YZ3aLAQsMA0VlWmy43r6FHqa/UN
339339
github.com/go-git/go-billy/v5 v5.6.2/go.mod h1:rcFC2rAsp/erv7CMz9GczHcuD0D32fWzH+MJAU+jaUU=
340340
github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399 h1:eMje31YglSBqCdIqdhKBW8lokaMrL3uTkpGYlE2OOT4=
341341
github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399/go.mod h1:1OCfN199q1Jm3HZlxleg+Dw/mwps2Wbk9frAWm+4FII=
342-
github.com/go-git/go-git/v5 v5.16.2 h1:fT6ZIOjE5iEnkzKyxTHK1W4HGAsPhqEqiSAssSO77hM=
343-
github.com/go-git/go-git/v5 v5.16.2/go.mod h1:4Ge4alE/5gPs30F2H1esi2gPd69R0C39lolkucHBOp8=
342+
github.com/go-git/go-git/v5 v5.16.3 h1:Z8BtvxZ09bYm/yYNgPKCzgWtaRqDTgIKRgIRHBfU6Z8=
343+
github.com/go-git/go-git/v5 v5.16.3/go.mod h1:4Ge4alE/5gPs30F2H1esi2gPd69R0C39lolkucHBOp8=
344344
github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU=
345345
github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8=
346346
github.com/go-ini/ini v1.67.0 h1:z6ZrTEZqSWOTyH2FlglNbNgARyHG8oLW9gMELqKr06A=
@@ -978,8 +978,8 @@ golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
978978
golang.org/x/sys v0.28.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
979979
golang.org/x/sys v0.29.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
980980
golang.org/x/sys v0.30.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
981-
golang.org/x/sys v0.36.0 h1:KVRy2GtZBrk1cBYA7MKu5bEZFxQk4NIDV6RLVcC8o0k=
982-
golang.org/x/sys v0.36.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks=
981+
golang.org/x/sys v0.37.0 h1:fdNQudmxPjkdUTPnLn5mdQv7Zwvbvpaxqs831goi9kQ=
982+
golang.org/x/sys v0.37.0/go.mod h1:OgkHotnGiDImocRcuBABYBEXf8A9a87e/uXjp9XT3ks=
983983
golang.org/x/telemetry v0.0.0-20240228155512-f48c80bd79b2/go.mod h1:TeRTkGYfJXctD9OcfyVLyj2J3IxLnKwHJR8f4D8a3YE=
984984
golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
985985
golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=

modules/httplib/request.go

Lines changed: 47 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -7,54 +7,53 @@ package httplib
77
import (
88
"bytes"
99
"context"
10-
"crypto/tls"
11-
"errors"
1210
"fmt"
1311
"io"
1412
"net"
1513
"net/http"
1614
"net/url"
1715
"strings"
16+
"sync"
1817
"time"
1918
)
2019

21-
var defaultSetting = Settings{"GiteaServer", 60 * time.Second, 60 * time.Second, nil, nil}
22-
23-
// newRequest returns *Request with specific method
24-
func newRequest(url, method string) *Request {
25-
var resp http.Response
26-
req := http.Request{
27-
Method: method,
28-
Header: make(http.Header),
29-
Proto: "HTTP/1.1",
30-
ProtoMajor: 1,
31-
ProtoMinor: 1,
20+
var defaultTransport = sync.OnceValue(func() http.RoundTripper {
21+
return &http.Transport{
22+
Proxy: http.ProxyFromEnvironment,
23+
DialContext: DialContextWithTimeout(10 * time.Second), // it is good enough in modern days
3224
}
33-
return &Request{url, &req, map[string]string{}, defaultSetting, &resp, nil}
34-
}
25+
})
3526

36-
// NewRequest returns *Request with specific method
37-
func NewRequest(url, method string) *Request {
38-
return newRequest(url, method)
27+
func DialContextWithTimeout(timeout time.Duration) func(ctx context.Context, network, address string) (net.Conn, error) {
28+
return func(ctx context.Context, network, address string) (net.Conn, error) {
29+
return (&net.Dialer{Timeout: timeout}).DialContext(ctx, network, address)
30+
}
3931
}
4032

41-
// Settings is the default settings for http client
42-
type Settings struct {
43-
UserAgent string
44-
ConnectTimeout time.Duration
45-
ReadWriteTimeout time.Duration
46-
TLSClientConfig *tls.Config
47-
Transport http.RoundTripper
33+
func NewRequest(url, method string) *Request {
34+
return &Request{
35+
url: url,
36+
req: &http.Request{
37+
Method: method,
38+
Header: make(http.Header),
39+
Proto: "HTTP/1.1", // FIXME: from legacy httplib, it shouldn't be hardcoded
40+
ProtoMajor: 1,
41+
ProtoMinor: 1,
42+
},
43+
params: map[string]string{},
44+
45+
// ATTENTION: from legacy httplib, callers must pay more attention to it, it will cause annoying bugs when the response takes a long time
46+
readWriteTimeout: 60 * time.Second,
47+
}
4848
}
4949

50-
// Request provides more useful methods for requesting one url than http.Request.
5150
type Request struct {
52-
url string
53-
req *http.Request
54-
params map[string]string
55-
setting Settings
56-
resp *http.Response
57-
body []byte
51+
url string
52+
req *http.Request
53+
params map[string]string
54+
55+
readWriteTimeout time.Duration
56+
transport http.RoundTripper
5857
}
5958

6059
// SetContext sets the request's Context
@@ -63,36 +62,24 @@ func (r *Request) SetContext(ctx context.Context) *Request {
6362
return r
6463
}
6564

66-
// SetTimeout sets connect time out and read-write time out for BeegoRequest.
67-
func (r *Request) SetTimeout(connectTimeout, readWriteTimeout time.Duration) *Request {
68-
r.setting.ConnectTimeout = connectTimeout
69-
r.setting.ReadWriteTimeout = readWriteTimeout
65+
// SetTransport sets the request transport, if not set, will use httplib's default transport with environment proxy support
66+
// ATTENTION: the http.Transport has a connection pool, so it should be reused as much as possible, do not create a lot of transports
67+
func (r *Request) SetTransport(transport http.RoundTripper) *Request {
68+
r.transport = transport
7069
return r
7170
}
7271

7372
func (r *Request) SetReadWriteTimeout(readWriteTimeout time.Duration) *Request {
74-
r.setting.ReadWriteTimeout = readWriteTimeout
73+
r.readWriteTimeout = readWriteTimeout
7574
return r
7675
}
7776

78-
// SetTLSClientConfig sets tls connection configurations if visiting https url.
79-
func (r *Request) SetTLSClientConfig(config *tls.Config) *Request {
80-
r.setting.TLSClientConfig = config
81-
return r
82-
}
83-
84-
// Header add header item string in request.
77+
// Header set header item string in request.
8578
func (r *Request) Header(key, value string) *Request {
8679
r.req.Header.Set(key, value)
8780
return r
8881
}
8982

90-
// SetTransport sets transport to
91-
func (r *Request) SetTransport(transport http.RoundTripper) *Request {
92-
r.setting.Transport = transport
93-
return r
94-
}
95-
9683
// Param adds query param in to request.
9784
// params build query string as ?key1=value1&key2=value2...
9885
func (r *Request) Param(key, value string) *Request {
@@ -125,11 +112,9 @@ func (r *Request) Body(data any) *Request {
125112
return r
126113
}
127114

128-
func (r *Request) getResponse() (*http.Response, error) {
129-
if r.resp.StatusCode != 0 {
130-
return r.resp, nil
131-
}
132-
115+
// Response executes request client and returns the response.
116+
// Caller MUST close the response body if no error occurs.
117+
func (r *Request) Response() (*http.Response, error) {
133118
var paramBody string
134119
if len(r.params) > 0 {
135120
var buf bytes.Buffer
@@ -160,59 +145,19 @@ func (r *Request) getResponse() (*http.Response, error) {
160145
return nil, err
161146
}
162147

163-
trans := r.setting.Transport
164-
if trans == nil {
165-
// create default transport
166-
trans = &http.Transport{
167-
TLSClientConfig: r.setting.TLSClientConfig,
168-
Proxy: http.ProxyFromEnvironment,
169-
DialContext: TimeoutDialer(r.setting.ConnectTimeout),
170-
}
171-
} else if t, ok := trans.(*http.Transport); ok {
172-
if t.TLSClientConfig == nil {
173-
t.TLSClientConfig = r.setting.TLSClientConfig
174-
}
175-
if t.DialContext == nil {
176-
t.DialContext = TimeoutDialer(r.setting.ConnectTimeout)
177-
}
178-
}
179-
180148
client := &http.Client{
181-
Transport: trans,
182-
Timeout: r.setting.ReadWriteTimeout,
183-
}
184-
185-
if len(r.setting.UserAgent) > 0 && len(r.req.Header.Get("User-Agent")) == 0 {
186-
r.req.Header.Set("User-Agent", r.setting.UserAgent)
149+
Transport: r.transport,
150+
Timeout: r.readWriteTimeout,
187151
}
188-
189-
resp, err := client.Do(r.req)
190-
if err != nil {
191-
return nil, err
152+
if client.Transport == nil {
153+
client.Transport = defaultTransport()
192154
}
193-
r.resp = resp
194-
return resp, nil
195-
}
196155

197-
// Response executes request client gets response manually.
198-
// Caller MUST close the response body if no error occurs
199-
func (r *Request) Response() (*http.Response, error) {
200-
if r == nil {
201-
return nil, errors.New("invalid request")
156+
if r.req.Header.Get("User-Agent") == "" {
157+
r.req.Header.Set("User-Agent", "GiteaHttpLib")
202158
}
203-
return r.getResponse()
204-
}
205159

206-
// TimeoutDialer returns functions of connection dialer with timeout settings for http.Transport Dial field.
207-
func TimeoutDialer(cTimeout time.Duration) func(ctx context.Context, net, addr string) (c net.Conn, err error) {
208-
return func(ctx context.Context, netw, addr string) (net.Conn, error) {
209-
d := net.Dialer{Timeout: cTimeout}
210-
conn, err := d.DialContext(ctx, netw, addr)
211-
if err != nil {
212-
return nil, err
213-
}
214-
return conn, nil
215-
}
160+
return client.Do(r.req)
216161
}
217162

218163
func (r *Request) GoString() string {

modules/lfstransfer/backend/backend.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ func (g *GiteaBackend) Batch(_ string, pointers []transfer.BatchItem, args trans
157157
}
158158

159159
// Download implements transfer.Backend. The returned reader must be closed by the caller.
160-
func (g *GiteaBackend) Download(oid string, args transfer.Args) (io.ReadCloser, int64, error) {
160+
func (g *GiteaBackend) Download(oid string, args transfer.Args) (_ io.ReadCloser, _ int64, retErr error) {
161161
idMapStr, exists := args[argID]
162162
if !exists {
163163
return nil, 0, ErrMissingID
@@ -188,7 +188,15 @@ func (g *GiteaBackend) Download(oid string, args transfer.Args) (io.ReadCloser,
188188
if err != nil {
189189
return nil, 0, fmt.Errorf("failed to get response: %w", err)
190190
}
191-
// no need to close the body here by "defer resp.Body.Close()", see below
191+
// We must return the ReaderCloser but not "ReadAll", to avoid OOM.
192+
// "transfer.Backend" will check io.Closer interface and close the Body reader.
193+
// So only close the Body when error occurs
194+
defer func() {
195+
if retErr != nil {
196+
_ = resp.Body.Close()
197+
}
198+
}()
199+
192200
if resp.StatusCode != http.StatusOK {
193201
return nil, 0, statusCodeToErr(resp.StatusCode)
194202
}
@@ -197,7 +205,6 @@ func (g *GiteaBackend) Download(oid string, args transfer.Args) (io.ReadCloser,
197205
if err != nil {
198206
return nil, 0, fmt.Errorf("failed to parse content length: %w", err)
199207
}
200-
// transfer.Backend will check io.Closer interface and close this Body reader
201208
return resp.Body, respSize, nil
202209
}
203210

0 commit comments

Comments
 (0)