Skip to content
This repository was archived by the owner on Jan 30, 2025. It is now read-only.

Commit 176800f

Browse files
inancgumusinancgumus
authored
Merge pull request #129 from grafana/fix/125-frame-session-ctx-cancelled
Fix/125 cannot create frame session (context canceled)
2 parents 85893f4 + 7987816 commit 176800f

16 files changed

+623
-311
lines changed

‎common/barrier_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ import (
3131
func TestBarrier(t *testing.T) {
3232
ctx := context.Background()
3333

34-
log := NewLogger(ctx, NullLogger(), false, nil)
34+
log := NewNullLogger()
3535

3636
timeoutSetings := NewTimeoutSettings(nil)
3737
frameManager := NewFrameManager(ctx, nil, nil, timeoutSetings, log)

‎common/browser.go

Lines changed: 63 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -85,21 +85,16 @@ type Browser struct {
8585
// NewBrowser creates a new browser
8686
func NewBrowser(ctx context.Context, cancelFn context.CancelFunc, browserProc *BrowserProcess, launchOpts *LaunchOptions, logger *Logger) (*Browser, error) {
8787
b := Browser{
88-
BaseEventEmitter: NewBaseEventEmitter(ctx),
89-
ctx: ctx,
90-
cancelFn: cancelFn,
91-
state: int64(BrowserStateOpen),
92-
browserProc: browserProc,
93-
conn: nil,
94-
connected: false,
95-
launchOpts: launchOpts,
96-
contexts: make(map[cdp.BrowserContextID]*BrowserContext),
97-
defaultContext: nil,
98-
pagesMu: sync.RWMutex{},
99-
pages: make(map[target.ID]*Page),
100-
sessionIDtoTargetIDMu: sync.RWMutex{},
101-
sessionIDtoTargetID: make(map[target.SessionID]target.ID),
102-
logger: logger,
88+
BaseEventEmitter: NewBaseEventEmitter(ctx),
89+
ctx: ctx,
90+
cancelFn: cancelFn,
91+
state: int64(BrowserStateOpen),
92+
browserProc: browserProc,
93+
launchOpts: launchOpts,
94+
contexts: make(map[cdp.BrowserContextID]*BrowserContext),
95+
pages: make(map[target.ID]*Page),
96+
sessionIDtoTargetID: make(map[target.SessionID]target.ID),
97+
logger: logger,
10398
}
10499
if err := b.connect(); err != nil {
105100
return nil, err
@@ -116,21 +111,27 @@ func (b *Browser) connect() error {
116111
}
117112

118113
b.connMu.Lock()
119-
defer b.connMu.Unlock()
120114
b.connected = true
115+
b.connMu.Unlock()
116+
117+
// We don't need to lock this because `connect()` is called only in NewBrowser
121118
b.defaultContext = NewBrowserContext(b.ctx, b.conn, b, "", NewBrowserContextOptions(), b.logger)
119+
122120
return b.initEvents()
123121
}
124122

125123
func (b *Browser) disposeContext(id cdp.BrowserContextID) error {
126124
b.logger.Debugf("Browser:disposeContext", "bctxid:%v", id)
125+
127126
action := target.DisposeBrowserContext(id)
128127
if err := action.Do(cdp.WithExecutor(b.ctx, b.conn)); err != nil {
129128
return fmt.Errorf("unable to dispose browser context %T: %w", action, err)
130129
}
130+
131131
b.contextsMu.Lock()
132132
defer b.contextsMu.Unlock()
133133
delete(b.contexts, id)
134+
134135
return nil
135136
}
136137

@@ -197,69 +198,84 @@ func (b *Browser) initEvents() error {
197198
}
198199

199200
func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) {
201+
evti := ev.TargetInfo
202+
200203
b.contextsMu.RLock()
201-
var browserCtx *BrowserContext = b.defaultContext
202-
bctx, ok := b.contexts[ev.TargetInfo.BrowserContextID]
204+
browserCtx := b.defaultContext
205+
bctx, ok := b.contexts[evti.BrowserContextID]
203206
if ok {
204207
browserCtx = bctx
205208
}
206-
b.logger.Debugf("Browser:onAttachedToTarget", "sid:%v tid:%v bctxid:%v bctx nil=%t", ev.SessionID, ev.TargetInfo.TargetID, ev.TargetInfo.BrowserContextID, bctx == nil)
207209
b.contextsMu.RUnlock()
208210

211+
b.logger.Debugf("Browser:onAttachedToTarget", "sid:%v tid:%v bctxid:%v bctx nil:%t",
212+
ev.SessionID, evti.TargetID, evti.BrowserContextID, browserCtx == nil)
213+
209214
// We're not interested in the top-level browser target, other targets or DevTools targets right now.
210-
isDevTools := strings.HasPrefix(ev.TargetInfo.URL, "devtools://devtools")
211-
if ev.TargetInfo.Type == "browser" || ev.TargetInfo.Type == "other" || isDevTools {
212-
b.logger.Debugf("Browser:onAttachedToTarget:return", "sid:%v tid:%v (devtools)", ev.SessionID, ev.TargetInfo.TargetID)
215+
isDevTools := strings.HasPrefix(evti.URL, "devtools://devtools")
216+
if evti.Type == "browser" || evti.Type == "other" || isDevTools {
217+
b.logger.Debugf("Browser:onAttachedToTarget:return", "sid:%v tid:%v (devtools)", ev.SessionID, evti.TargetID)
213218
return
214219
}
215220

216-
if ev.TargetInfo.Type == "background_page" {
217-
p, err := NewPage(b.ctx, b.conn.getSession(ev.SessionID), browserCtx, ev.TargetInfo.TargetID, nil, false, b.logger)
221+
switch evti.Type {
222+
case "background_page":
223+
p, err := NewPage(b.ctx, b.conn.getSession(ev.SessionID), browserCtx, evti.TargetID, nil, false, b.logger)
218224
if err != nil {
219225
isRunning := atomic.LoadInt64(&b.state) == BrowserStateOpen && b.IsConnected() //b.conn.isConnected()
220226
if _, ok := err.(*websocket.CloseError); !ok && !isRunning {
221227
// If we're no longer connected to browser, then ignore WebSocket errors
222228
b.logger.Debugf("Browser:onAttachedToTarget:background_page:return", "sid:%v tid:%v websocket err:%v",
223-
ev.SessionID, ev.TargetInfo.TargetID, err)
229+
ev.SessionID, evti.TargetID, err)
224230
return
225231
}
226232
k6Throw(b.ctx, "cannot create NewPage for background_page event: %w", err)
227233
}
234+
228235
b.pagesMu.Lock()
229-
b.logger.Debugf("Browser:onAttachedToTarget:background_page:addTid", "sid:%v tid:%v", ev.SessionID, ev.TargetInfo.TargetID)
230-
b.pages[ev.TargetInfo.TargetID] = p
236+
b.logger.Debugf("Browser:onAttachedToTarget:background_page:addTid", "sid:%v tid:%v", ev.SessionID, evti.TargetID)
237+
b.pages[evti.TargetID] = p
231238
b.pagesMu.Unlock()
239+
232240
b.sessionIDtoTargetIDMu.Lock()
233-
b.logger.Debugf("Browser:onAttachedToTarget:background_page:addSid", "sid:%v tid:%v", ev.SessionID, ev.TargetInfo.TargetID)
234-
b.sessionIDtoTargetID[ev.SessionID] = ev.TargetInfo.TargetID
241+
b.logger.Debugf("Browser:onAttachedToTarget:background_page:addSid", "sid:%v tid:%v", ev.SessionID, evti.TargetID)
242+
b.sessionIDtoTargetID[ev.SessionID] = evti.TargetID
235243
b.sessionIDtoTargetIDMu.Unlock()
236-
} else if ev.TargetInfo.Type == "page" {
244+
case "page":
237245
var opener *Page = nil
238246
b.pagesMu.RLock()
239-
if t, ok := b.pages[ev.TargetInfo.OpenerID]; ok {
247+
if t, ok := b.pages[evti.OpenerID]; ok {
240248
opener = t
241249
}
242-
b.logger.Debugf("Browser:onAttachedToTarget:page", "sid:%v tid:%v opener nil:%t", ev.SessionID, ev.TargetInfo.TargetID, opener == nil)
250+
b.logger.Debugf("Browser:onAttachedToTarget:page", "sid:%v tid:%v opener nil:%t", ev.SessionID, evti.TargetID, opener == nil)
243251
b.pagesMu.RUnlock()
244-
p, err := NewPage(b.ctx, b.conn.getSession(ev.SessionID), browserCtx, ev.TargetInfo.TargetID, opener, true, b.logger)
252+
253+
p, err := NewPage(b.ctx, b.conn.getSession(ev.SessionID), browserCtx, evti.TargetID, opener, true, b.logger)
245254
if err != nil {
246255
isRunning := atomic.LoadInt64(&b.state) == BrowserStateOpen && b.IsConnected() //b.conn.isConnected()
247256
if _, ok := err.(*websocket.CloseError); !ok && !isRunning {
248257
// If we're no longer connected to browser, then ignore WebSocket errors
249-
b.logger.Debugf("Browser:onAttachedToTarget:page:return", "sid:%v tid:%v websocket err:", ev.SessionID, ev.TargetInfo.TargetID)
258+
b.logger.Debugf("Browser:onAttachedToTarget:page:return", "sid:%v tid:%v websocket err:", ev.SessionID, evti.TargetID)
250259
return
251260
}
252261
k6Throw(b.ctx, "cannot create NewPage for page event: %w", err)
253262
}
263+
254264
b.pagesMu.Lock()
255-
b.logger.Debugf("Browser:onAttachedToTarget:page:addTarget", "sid:%v tid:%v", ev.SessionID, ev.TargetInfo.TargetID)
256-
b.pages[ev.TargetInfo.TargetID] = p
265+
b.logger.Debugf("Browser:onAttachedToTarget:page:addTarget", "sid:%v tid:%v", ev.SessionID, evti.TargetID)
266+
b.pages[evti.TargetID] = p
257267
b.pagesMu.Unlock()
268+
258269
b.sessionIDtoTargetIDMu.Lock()
259-
b.logger.Debugf("Browser:onAttachedToTarget:page:sidToTid", "sid:%v tid:%v", ev.SessionID, ev.TargetInfo.TargetID)
260-
b.sessionIDtoTargetID[ev.SessionID] = ev.TargetInfo.TargetID
270+
b.logger.Debugf("Browser:onAttachedToTarget:page:sidToTid", "sid:%v tid:%v", ev.SessionID, evti.TargetID)
271+
b.sessionIDtoTargetID[ev.SessionID] = evti.TargetID
261272
b.sessionIDtoTargetIDMu.Unlock()
273+
262274
browserCtx.emit(EventBrowserContextPage, p)
275+
default:
276+
b.logger.Warnf(
277+
"Browser:onAttachedToTarget", "sid:%v tid:%v bctxid:%v bctx nil:%t, unknown target type: %q",
278+
ev.SessionID, evti.TargetID, evti.BrowserContextID, browserCtx == nil, evti.Type)
263279
}
264280
}
265281

@@ -346,16 +362,21 @@ func (b *Browser) Close() {
346362
b.logger.Debugf("Browser:Close", "already in a closing state")
347363
return
348364
}
349-
b.browserProc.GracefulClose()
350-
defer b.browserProc.Terminate()
365+
366+
atomic.CompareAndSwapInt64(&b.state, b.state, BrowserStateClosed)
351367

352368
action := cdpbrowser.Close()
353369
if err := action.Do(cdp.WithExecutor(b.ctx, b.conn)); err != nil {
354370
if _, ok := err.(*websocket.CloseError); !ok {
355371
k6Throw(b.ctx, "unable to execute %T: %v", action, err)
356372
}
357373
}
358-
atomic.CompareAndSwapInt64(&b.state, b.state, BrowserStateClosed)
374+
375+
// terminate the browser process early on, then tell the CDP
376+
// afterwards. this will take a little bit of time, and CDP
377+
// will stop emitting events.
378+
b.browserProc.GracefulClose()
379+
b.browserProc.Terminate()
359380
}
360381

361382
// Contexts returns list of browser contexts
@@ -374,6 +395,7 @@ func (b *Browser) Contexts() []api.BrowserContext {
374395
func (b *Browser) IsConnected() bool {
375396
b.connMu.RLock()
376397
defer b.connMu.RUnlock()
398+
377399
return b.connected
378400
}
379401

‎common/connection.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,9 @@ func (c *Connection) send(msg *cdproto.Message, recvCh chan *cdproto.Message, re
358358
case <-c.done:
359359
c.logger.Debugf("Connection:send:<-c.done", "wsURL:%q sid:%v", c.wsURL, msg.SessionID)
360360
return nil
361+
case <-c.ctx.Done():
362+
c.logger.Errorf("Connection:send:<-c.ctx.Done()", "wsURL:%q sid:%v err:%v", c.wsURL, msg.SessionID, c.ctx.Err())
363+
return nil
361364
}
362365

363366
// Block waiting for response.

‎common/connection_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func TestConnection(t *testing.T) {
4444
ctx := context.Background()
4545
url, _ := url.Parse(server.ServerHTTP.URL)
4646
wsURL := fmt.Sprintf("ws://%s/echo", url.Host)
47-
conn, err := NewConnection(ctx, wsURL, NewLogger(ctx, NullLogger(), false, nil))
47+
conn, err := NewConnection(ctx, wsURL, NewNullLogger())
4848
conn.Close()
4949

5050
require.NoError(t, err)
@@ -58,7 +58,7 @@ func TestConnectionClosureAbnormal(t *testing.T) {
5858
ctx := context.Background()
5959
url, _ := url.Parse(server.ServerHTTP.URL)
6060
wsURL := fmt.Sprintf("ws://%s/closure-abnormal", url.Host)
61-
conn, err := NewConnection(ctx, wsURL, NewLogger(ctx, NullLogger(), false, nil))
61+
conn, err := NewConnection(ctx, wsURL, NewNullLogger())
6262

6363
if assert.NoError(t, err) {
6464
action := target.SetDiscoverTargets(true)
@@ -75,7 +75,7 @@ func TestConnectionSendRecv(t *testing.T) {
7575
ctx := context.Background()
7676
url, _ := url.Parse(server.ServerHTTP.URL)
7777
wsURL := fmt.Sprintf("ws://%s/cdp", url.Host)
78-
conn, err := NewConnection(ctx, wsURL, NewLogger(ctx, NullLogger(), false, nil))
78+
conn, err := NewConnection(ctx, wsURL, NewNullLogger())
7979

8080
if assert.NoError(t, err) {
8181
action := target.SetDiscoverTargets(true)
@@ -138,7 +138,7 @@ func TestConnectionCreateSession(t *testing.T) {
138138
ctx := context.Background()
139139
url, _ := url.Parse(server.ServerHTTP.URL)
140140
wsURL := fmt.Sprintf("ws://%s/cdp", url.Host)
141-
conn, err := NewConnection(ctx, wsURL, NewLogger(ctx, NullLogger(), false, nil))
141+
conn, err := NewConnection(ctx, wsURL, NewNullLogger())
142142

143143
if assert.NoError(t, err) {
144144
session, err := conn.createSession(&target.Info{

0 commit comments

Comments
 (0)