Remove buffer pool#1336
Conversation
There was a problem hiding this comment.
Pull request overview
Removes object pooling (buffers, build tasks, and fetch clients) across the server and updates call sites accordingly, simplifying lifecycle management and eliminating recycle/defer patterns.
Changes:
- Replaced
newBuffer()(pooledbytes.Buffer) usages with direct&bytes.Buffer{}/bytes.NewBufferString(...)allocations. - Removed
BuildTaskpooling inBuildQueueand switched to allocating tasks per request. - Simplified
internal/fetch.NewClientby removing client pooling and changing its signature (and removing the optional host allowlist logic).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/server.go | Updates custom landing page fetch client creation to new fetch.NewClient signature. |
| server/router.go | Replaces pooled buffers with direct bytes.Buffer allocations and updates fetch client creation. |
| server/path.go | Updates PR version resolver to new fetch.NewClient signature (removes recycle defer). |
| server/npmrc.go | Updates registry fetch client creation to new fetch.NewClient signature. |
| server/legacy_router.go | Updates legacy fetch client creation to new fetch.NewClient signature. |
| server/git.go | Replaces pooled buffers used for exec.Cmd output capture; updates fetch client creation. |
| server/dts_transform.go | Switches DTS transform buffer creation to direct bytes.Buffer. |
| server/dts_lexer.go | Switches temporary buffer creation to direct bytes.Buffer. |
| server/cjs_module_lexer.go | Replaces pooled stdout/stderr buffers with direct bytes.Buffer allocations. |
| server/cache.go | Removes the global bufferPool and newBuffer() helper entirely. |
| server/build_queue.go | Removes BuildTask pooling and task recycling logic; allocates tasks directly. |
| server/build.go | Replaces pooled buffers with direct bytes.Buffer allocations when assembling output. |
| internal/fetch/fetch.go | Removes FetchClient pooling, changes NewClient signature, and removes host allowlist logic. |
Comments suppressed due to low confidence (1)
server/build_queue.go:181
- These comments still refer to "recycling" the task and to
task.ctxpotentially being nil due to recycling, but the pooling/recycle logic was removed. Please update or remove the comments to reflect the new lifecycle so future readers aren't misled.
// store wait channels before recycling the task
waitChans := task.waitChans
// send the build output
output := BuildOutput{meta, err}
for _, ch := range waitChans {
if ch != nil {
select {
case ch <- output:
// successfully sent
default:
// channel is full or closed, log the drop
// Note: task.ctx might be nil here due to recycling, so we can't log the path
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NewClient creates a new FetchClient. | ||
| func NewClient(userAgent string, timeout int, reserveRedirect bool, allowedHosts map[string]struct{}) (client *FetchClient, recycle func()) { | ||
| client = clientPool.Get().(*FetchClient) | ||
| func NewClient(userAgent string, timeout int, reserveRedirect bool) (client *FetchClient) { | ||
| client = &FetchClient{Client: &http.Client{}} | ||
| client.userAgent = userAgent | ||
| client.Timeout = time.Duration(timeout) * time.Second | ||
| client.CheckRedirect = func(req *http.Request, via []*http.Request) error { | ||
| if reserveRedirect && len(via) > 0 { | ||
| return http.ErrUseLastResponse | ||
| } | ||
| // To avoid SSRF attacks, we check if the request URL's host is in the allowed hosts list. | ||
| if allowedHosts != nil { | ||
| if _, ok := allowedHosts[req.URL.Host]; !ok { | ||
| return http.ErrUseLastResponse | ||
| } | ||
| } | ||
| if len(via) >= 6 { | ||
| return errors.New("too many redirects") | ||
| } | ||
| return nil | ||
| } | ||
| return client, func() { clientPool.Put(client) } | ||
| return client |
There was a problem hiding this comment.
PR title indicates buffer pool removal, but this hunk also changes the internal/fetch API and removes the (optional) host allowlist/redirect restrictions. If this is intentional, it should be called out explicitly in the PR description/title since it changes security-related behavior and the fetch package surface area.
| // NewClient creates a new FetchClient. | ||
| func NewClient(userAgent string, timeout int, reserveRedirect bool, allowedHosts map[string]struct{}) (client *FetchClient, recycle func()) { | ||
| client = clientPool.Get().(*FetchClient) | ||
| func NewClient(userAgent string, timeout int, reserveRedirect bool) (client *FetchClient) { | ||
| client = &FetchClient{Client: &http.Client{}} | ||
| client.userAgent = userAgent | ||
| client.Timeout = time.Duration(timeout) * time.Second | ||
| client.CheckRedirect = func(req *http.Request, via []*http.Request) error { | ||
| if reserveRedirect && len(via) > 0 { | ||
| return http.ErrUseLastResponse | ||
| } | ||
| // To avoid SSRF attacks, we check if the request URL's host is in the allowed hosts list. | ||
| if allowedHosts != nil { | ||
| if _, ok := allowedHosts[req.URL.Host]; !ok { | ||
| return http.ErrUseLastResponse | ||
| } | ||
| } | ||
| if len(via) >= 6 { | ||
| return errors.New("too many redirects") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
NewClient no longer accepts/records an allowedHosts allowlist, and the host checks in both CheckRedirect and Fetch were removed. This drops the only built-in host restriction mechanism in internal/fetch (previously used as an SSRF mitigation); if host allowlisting is still desired anywhere, consider restoring this option (or adding an explicit host validation layer at call sites / a separate "restricted" client constructor).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.