-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix/49 cannot find context with specified id #169
Conversation
3b87092
to
63041e8
Compare
6b9a264
to
7e4eb83
Compare
Sometimes sessions get closed and they clog the communication between frame sessions and sessions. This usually happens when a frame session is getting created after new frame attachments. This was causing problems with frame handling. This commit prevents lagging by failing fast when it detects a session is closed. It also changes the ordering of FrameSession init steps. The reason of putting initOptions last is: It was propagating frame events before the frame is ready to be used.
7e4eb83
to
8c7fc3b
Compare
1fb4d49
to
bce2f54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer: I haven't run the code, just looked at it 🙂
You mention in one of the commit comments:
It also changes the ordering of FrameSession
init steps. The reason of putting initOptions
last is: It was propagating frame events
before the frame is ready to be used.
Could you share more info here, what was being propagated by initOptions
? From what I know it's the enabling of domains and the target.SetAutoAttach
call in initDomains
that triggers the browser to report all frames and execution contexts etc.
@robingustafsson, yes, you're right: It triggers a lot of things. That's why I put it last for safety. I wanted a Fail-fast fix: What this fix is all about is to terminate Lines 68 to 77 in bce2f54
Line 181 in bce2f54
Line 226 in bce2f54
xk6-browser/common/frame_session.go Lines 245 to 248 in bce2f54
xk6-browser/common/frame_session.go Lines 840 to 842 in bce2f54
|
Ah wait, I see now in the code that you actually put Thanks for clarifying the fail-fast fix as well, I see now that it's the "auto-cancellation" of the context when |
Yeah, my bad, sorry 🤦 Yes, exactly, that's what I mean by failing-fast 😄 |
@robingustafsson Does that mean you approve the PR? :) Can I include this fix in Friday's release? |
Yes, it's good to go (I can't seem to change my review to approve) 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self TODO.
case <-c.ctx.Done(): | ||
c.logger.Errorf("Connection:send:<-c.ctx.Done()", "wsURL:%q sid:%v err:%v", c.wsURL, msg.SessionID, c.ctx.Err()) | ||
return nil | ||
return ctx.Err() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.ctx.Err()
// Erroneous cases | ||
defer fs.logger.Debugf("FrameSession:onAttachedToTarget:NewFrameSession", | ||
"sid:%v tid:%v esid:%v etid:%v ebctxid:%v type:%q err:%v", | ||
if session == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here that explains why the session can be nil.
err = fs.attachWorkerToTarget(ti, sid) | ||
default: | ||
// Just unblock (debugger continue) these targets and detach from them. | ||
s := fs.page.browserCtx.conn.getSession(sid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment that says the session may get lost interim.
default: | ||
// Just unblock (debugger continue) these targets and detach from them. | ||
s := fs.page.browserCtx.conn.getSession(sid) | ||
_ = s.ExecuteWithoutExpectationOnReply(fs.ctx, runtime.CommandRunIfWaitingForDebugger, nil, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check s.close here?
if err == nil { | ||
return | ||
} | ||
// Handle or ignore errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this IsIgnorableFrameSessionErr?
This PR fixes #49 for good 🎉
After long debugging sessions and trials, finally, I figured out the problem:
FrameSession
was blocking frame handling, and the rest of the system couldn't propagate execution contexts, causing thecontext with specified id not found
error. This misbehavior happens because aSession
can get closed while aFrameSession
is in the initialization stage.The only error we are receiving is different now:
wait for selector didn't result in any nodes
on theClick Continue (Recommendations)
group. I tested this patch with Chromium 98.0.4753.0. I also tested this with other test runners, and they stuck on the same step. I'm going to create a new issue for it.Here's the script I'm using. It's a more concise and faster (_pauseMin/Max_) version of Tom's original script.
Here's Tom's original script that also runs without context errors. I just added a `waitForNavigation` call before `page.waitForSelector("//span[text()='Step 4']");`.
Here's another script Tom has shared on Jan 4th, 2022. I modified it a little bit to wait for navigation.