From d79058d8687436f5113b6d1d97c169fe1db0f757 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Fri, 11 Oct 2024 23:26:37 +0100 Subject: [PATCH 1/2] Fix memory leak with req->resp->req ref chain We need to ensure that the reference from request to response and back to request is broken by nilling them out. This then allows the GC to correctly collect the objects and free memory. --- common/network_manager.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/common/network_manager.go b/common/network_manager.go index 1daba3054..c65e52923 100644 --- a/common/network_manager.go +++ b/common/network_manager.go @@ -179,9 +179,27 @@ func parseTTL(ttlS string) (time.Duration, error) { } func (m *NetworkManager) deleteRequestByID(reqID network.RequestID) { + var req *Request m.reqsMu.Lock() - defer m.reqsMu.Unlock() + req = m.reqIDToRequest[reqID] delete(m.reqIDToRequest, reqID) + m.reqsMu.Unlock() + + // We need to nil the request in response otherwise they hold onto each + // others reference preventing the GC from cleaning the memory up. + req.responseMu.Lock() + if req.response != nil { + req.response.request = nil + } + req.response = nil + req.responseMu.Unlock() + + for _, r := range req.redirectChain { + if reqID != r.getID() { + m.deleteRequestByID(r.getID()) + } + } + req.redirectChain = nil } func (m *NetworkManager) emitRequestMetrics(req *Request) { From 658063afe65f09cdf2b2ed78cce7b51e4c145d38 Mon Sep 17 00:00:00 2001 From: ankur22 Date: Sat, 12 Oct 2024 19:01:11 +0100 Subject: [PATCH 2/2] Add nil to references to mark as ready to be GC'd This is a POC to understand why memory isn't being deallocated normally without having to do this. --- common/browser.go | 13 +++++++++++++ common/browser_context.go | 11 +++++++++++ common/connection.go | 11 +++++++++++ common/frame.go | 13 +++++++++++++ common/frame_manager.go | 17 +++++++++++++++++ common/frame_session.go | 18 ++++++++++++++++++ common/js_handle.go | 4 ++++ common/network_manager.go | 18 ++++++++++++++++++ common/page.go | 23 +++++++++++++++++++++++ common/session.go | 2 ++ 10 files changed, 130 insertions(+) diff --git a/common/browser.go b/common/browser.go index 8a1638caf..1a53dd38a 100644 --- a/common/browser.go +++ b/common/browser.go @@ -223,6 +223,19 @@ func (b *Browser) initEvents() error { //nolint:cyclop if b.vuCtxCancelFn != nil { b.vuCtxCancelFn() } + + go func() { + <-b.browserCtx.Done() + + b.browserProc = nil + b.browserOpts = nil + b.conn = nil + b.context = nil + b.defaultContext = nil + b.pages = nil + b.sessionIDtoTargetID = nil + b.logger = nil + }() }() for { select { diff --git a/common/browser_context.go b/common/browser_context.go index b2a0ef8ab..02195adeb 100644 --- a/common/browser_context.go +++ b/common/browser_context.go @@ -120,6 +120,17 @@ func NewBrowserContext( return nil, fmt.Errorf("adding web vital init script to new browser context: %w", err) } + go func() { + <-b.ctx.Done() + + b.browser = nil + b.opts = nil + b.timeoutSettings = nil + b.logger = nil + b.vu = nil + b.evaluateOnNewDocumentSources = nil + }() + return &b, nil } diff --git a/common/connection.go b/common/connection.go index 27a2a119c..4c415cc4d 100644 --- a/common/connection.go +++ b/common/connection.go @@ -199,6 +199,17 @@ func (c *Connection) close(code int) error { // Stop the main control loop close(c.done) _ = c.conn.Close() + + go func() { + <-c.ctx.Done() + + c.cancelCtx = nil + c.logger = nil + c.conn = nil + c.msgIDGen = nil + c.sessions = nil + c.onTargetAttachedToTarget = nil + }() }() c.closeAllSessions() diff --git a/common/frame.go b/common/frame.go index 51e44855f..99d2ab781 100644 --- a/common/frame.go +++ b/common/frame.go @@ -248,6 +248,19 @@ func (f *Frame) detach() error { return fmt.Errorf("disposing document handle while detaching frame: %w", err) } + f.page = nil + f.manager = nil + f.parentFrame = nil + f.childFrames = nil + f.vu = nil + f.lifecycleEvents = nil + f.documentHandle = nil + f.executionContexts = nil + f.inflightRequests = nil + f.currentDocument = nil + f.pendingDocument = nil + f.log = nil + return nil } diff --git a/common/frame_manager.go b/common/frame_manager.go index 617f20f9e..1a6d022d1 100644 --- a/common/frame_manager.go +++ b/common/frame_manager.go @@ -66,6 +66,23 @@ func NewFrameManager( id: atomic.AddInt64(&frameManagerID, 1), } + go func() { + <-m.ctx.Done() + + for _, f := range m.frames { + m.removeFramesRecursively(f) + } + + m.session = nil + m.page = nil + m.timeoutSettings = nil + m.mainFrame = nil + m.frames = nil + m.barriers = nil + m.vu = nil + m.logger = nil + }() + m.logger.Debugf("FrameManager:New", "fmid:%d", m.ID()) return m diff --git a/common/frame_session.go b/common/frame_session.go index f669e797f..b79f5012f 100644 --- a/common/frame_session.go +++ b/common/frame_session.go @@ -236,6 +236,24 @@ func (fs *FrameSession) initEvents() { } go func() { + go func() { + <-fs.ctx.Done() + + fs.session = nil + fs.page = nil + fs.parent = nil + fs.manager = nil + fs.networkManager = nil + fs.k6Metrics = nil + fs.contextIDToContext = nil + fs.isolatedWorlds = nil + fs.eventCh = nil + fs.childSessions = nil + fs.vu = nil + fs.logger = nil + fs.mainFrameSpan = nil + }() + fs.logger.Debugf("NewFrameSession:initEvents:go", "sid:%v tid:%v", fs.session.ID(), fs.targetID) defer func() { diff --git a/common/js_handle.go b/common/js_handle.go index a33cf054f..2f5961e10 100644 --- a/common/js_handle.go +++ b/common/js_handle.go @@ -119,6 +119,10 @@ func (h *BaseJSHandle) dispose() error { h.remoteObject.ObjectID, err) } + h.session = nil + h.execCtx = nil + h.remoteObject = nil + return nil } diff --git a/common/network_manager.go b/common/network_manager.go index c65e52923..8369aa0c2 100644 --- a/common/network_manager.go +++ b/common/network_manager.go @@ -190,10 +190,13 @@ func (m *NetworkManager) deleteRequestByID(reqID network.RequestID) { req.responseMu.Lock() if req.response != nil { req.response.request = nil + req.response.body = nil } req.response = nil req.responseMu.Unlock() + req.postDataEntries = nil + for _, r := range req.redirectChain { if reqID != r.getID() { m.deleteRequestByID(r.getID()) @@ -372,6 +375,21 @@ func (m *NetworkManager) initEvents() { }, chHandler) go func() { + defer func() { + m.logger = nil + m.session = nil + m.parent = nil + m.frameManager = nil + m.credentials = nil + m.resolver = nil + m.vu = nil + m.customMetrics = nil + m.mi = nil + m.reqIDToRequest = nil + m.attemptedAuth = nil + m.extraHTTPHeaders = nil + }() + for m.handleEvents(chHandler) { } }() diff --git a/common/page.go b/common/page.go index 0cfa718e2..5e1091ad6 100644 --- a/common/page.go +++ b/common/page.go @@ -348,6 +348,29 @@ func (p *Page) initEvents() { defer func() { p.logger.Debugf("Page:initEvents:go:return", "sid:%v tid:%v", p.session.ID(), p.targetID) + + go func() { + <-p.ctx.Done() + + p.Keyboard = nil + p.Mouse = nil + p.Touchscreen = nil + p.session = nil + p.browserCtx = nil + p.opener = nil + p.frameManager = nil + p.timeoutSettings = nil + p.emulatedSize = nil + p.extraHTTPHeaders = nil + p.eventCh = nil + p.eventHandlers = nil + p.mainFrameSession = nil + p.frameSessions = nil + p.workers = nil + p.routes = nil + p.vu = nil + p.logger = nil + }() }() for { diff --git a/common/session.go b/common/session.go index 50326aafe..0cc08208c 100644 --- a/common/session.go +++ b/common/session.go @@ -70,6 +70,8 @@ func (s *Session) close() { s.closed = true s.emit(EventSessionClosed, nil) + + s.conn = nil } func (s *Session) markAsCrashed() {