Skip to content
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

browser: get session only once - trying to fix #4125 #4539

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mstoykov
Copy link
Contributor

What?

Use the session we get a bit earlier in attachIFrameToTarget instead of regetting it there.

Why?

This should fix #4125 as we already check the session isn't nil at the time.

It is still strange it suddenly disappears, and my expectation is that it is likely something to do with k6 stopping and session going away.

Also reuse the session we get in other cases within onAttachedToTarget as we already have it.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@mstoykov mstoykov added this to the v1.0.0-rc1 milestone Feb 11, 2025
@mstoykov mstoykov requested a review from a team as a code owner February 11, 2025 16:07
@mstoykov mstoykov requested review from inancgumus and joanlopez and removed request for a team February 11, 2025 16:07
@mstoykov
Copy link
Contributor Author

@inancgumus you are probably the person who can currently say if this is good idea at all.

Unfortunately I do not have a reproducible example that we can test with.

@inancgumus
Copy link
Member

inancgumus commented Feb 11, 2025

@inancgumus you are probably the person who can currently say if this is good idea at all.

Unfortunately I do not have a reproducible example that we can test with.

This change may solve the nil-pointer issue 🤞 However, the racy conditions will remain. Sessions in Connection are protected by a mutex. We get the session by locking and unlocking the mutex. After getting the session, while onAttachedToTarget is working, in the meantime, Connection may delete the session. Communicating over that Session can result in unexpected errors (because it may be gone from the browser and Connection side). The session management in the browser should definitely be improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPD in NewFrameSession
2 participants