perf: fix mutex contention, double deserialization, allocation, and goroutine leak - #105
Conversation
…oroutine leak - cmdutil/line_writer: release mutex before blocking I/O and handler calls (#67) - fileutil: validate cache metadata before full deserialization (#68) - httpclient: reuse http.Client via context-based redirect config (#69) - httpclient: use already-parsed maps in pagination instead of re-unmarshaling (#70) - progress: guard Start() against multiple calls to prevent goroutine leaks (#71) Fixes #67, #68, #69, #70, #71 Co-authored-by: GitHub Copilot <[email protected]>
df1663b to
9bdb211
Compare
jongio
left a comment
There was a problem hiding this comment.
Two correctness issues and a CI fix needed:
- cmdutil/line_writer.go: write error path loses buffered lines (see inline).
- httpclient/client.go: pagination requests don't carry the redirect config through context. handlePagination receives the raw ctx, not req.Context() which has the injected redirectConfig. Means FollowRedirects=true won't work for pagination link fetches. Fix: pass req.Context() instead of ctx to handlePagination.
- CI lint failure: the type definitions inserted between the // NewClient doc comment and func NewClient(...) detach the doc comment from the function. Move the types above the comment or repeat the doc comment adjacent to the func.
| if output != nil { | ||
| n, err = output.Write(p) | ||
| if err != nil { | ||
| return n, err |
There was a problem hiding this comment.
If output.Write(p) fails here, the function returns early but lines were already extracted from lw.buf at line 40. Those lines are gone from the buffer and handlers never fire.
Original code wrote to output before buffering, so a write failure meant nothing was consumed. This version buffers first, then writes.
Fix: move the output write before the buffer manipulation (still outside the lock). Sequence should be:
- Lock, capture refs, release lock
- output.Write(p) - fail early, nothing consumed yet
- Lock, buffer + extract lines, release lock
- Call handlers
| }, | ||
| maxRedirects = 10 // Default max redirects | ||
| } | ||
| client := c.httpClient |
There was a problem hiding this comment.
handlePagination(ctx, client, opts, response) at ~line 291 receives ctx (the original function parameter), not req.Context() (which carries the redirectConfig). Since client is now the shared client with context-based CheckRedirect, pagination requests won't have the redirect config and will default to blocking all redirects.
Probably not an issue in practice (Azure pagination URLs don't redirect), but it's a semantic regression from the original code where the per-call client closure captured opts.FollowRedirects.
Fix: handlePagination(req.Context(), client, opts, response)
Summary
Fix 5 performance issues across 4 packages:
cmdutil/line_writer (perf(cmdutil): lineWriter holds mutex across blocking I/O and callbacks #67): Release mutex before blocking I/O - copies output ref and handler under lock, extracts complete lines, then performs writes outside the critical section.
fileutil (perf(fileutil): LoadCacheJSON deserializes same data twice #68): Validate cache metadata before full deserialization - unmarshal lightweight metadata first, skip expensive target unmarshal when cache is expired.
httpclient (perf(httpclient): new http.Client allocated on every Execute() call #69): Reuse http.Client via context-based redirect config - single shared client with CheckRedirect reading per-request policy from context.WithValue instead of allocating a new http.Client per Execute call.
httpclient (perf(httpclient): pagination unmarshals each page body twice #70): Use already-parsed maps in pagination - added extractNextLinkFromParsed that operates on map[string]any, avoiding redundant json.Unmarshal when the body is already deserialized.
progress (perf(progress): goroutine leak when Start() called without Stop() #71): Guard Start() against multiple calls - added started field to prevent goroutine leaks when Start is called more than once without Stop.
Testing
All existing tests pass across all 4 affected packages.
Fixes #67, #68, #69, #70, #71