Skip to content

fix: Revert - fix(session): Always have calling session be the parent session when making scoped session #1924

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 17, 2025
Merged

fix: Revert - fix(session): Always have calling session be the parent session when making scoped session #1924

merged 1 commit into from
Mar 17, 2025

Conversation

@schloerke
Copy link
Collaborator

@schloerke schloerke commented Mar 17, 2025 •
edited
Loading

This reverts commit 14b92d4 via #1923

From conversations with @jcheng5 , the proxy sessions should have a single "parent" object which is "Root" session, they should not recurse up the session family tree during calls.

In a followup PR, I'll rename ._parent to ._root_session to avoid confusion.

From chat w/ @jcheng5

If there are reasons we want nested session scopes to actually be related (and there are reasons we might--like we could add .destroy() methods that recursively kill their child scopes), we can do that, but we’d have to make further changes I think.

Off the top of my head, we’d have to make sure that make_scope:

  1. Always returns the same object when make_scope is passed with the same id, much like Python does with loaded modules. We’d need to do this with weakrefs.
  2. If make_scope is passed with an id more than one level deep, we’d need to create/fetch a separate session proxy object for each level.

@schloerke
Revert "fix(session): Always have calling session be the parent sessi…
7757a92 
…on when making a scoped session (#1923)"

This reverts commit 14b92d4.
@schloerke schloerke changed the title revert: fix(session): Always have calling session be the parent session when making scoped session fix: Revert - fix(session): Always have calling session be the parent session when making scoped session Mar 17, 2025
@schloerke schloerke requested review from Copilot and jcheng5 March 17, 2025 17:32
Copilot AI reviewed Mar 17, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR reverts a previous change to how scoped sessions are created so that the calling session is always used as the parent session. Key changes include:

  • Reverting the use of SessionProxy in favor of delegating the make_scope call to the parent session.
  • Updating the CHANGELOG to remove the entry for the previous behavior.
  • Removing a test skip that previously disabled recursive session tests.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
shiny/session/_session.py Reverts make_scope to call parent's make_scope method
CHANGELOG.md Removes outdated changelog entry for the reverted fix
tests/playwright/shiny/bookmark/modules/test_bookmark_modules.py Removes skip for recursive test
Comments suppressed due to low confidence (2)

shiny/session/_session.py:1237

  • Verify that deferring make_scope entirely to the parent session does not bypass necessary logic from the previously used SessionProxy.
return self._parent.make_scope(self.ns(id))

tests/playwright/shiny/bookmark/modules/test_bookmark_modules.py:28

  • Ensure that the removal of the recursive test skip is intentional and that recursive session behavior is now correctly handled to avoid potential test failures.
if "recursive" in app_name:

@schloerke schloerke marked this pull request as ready for review March 17, 2025 17:33
@jcheng5
Copy link
Collaborator

jcheng5 commented Mar 17, 2025

LGTM if @wch approves

@schloerke schloerke requested a review from wch March 17, 2025 17:37
@schloerke
Copy link
Collaborator Author

schloerke commented Mar 17, 2025

Approval received via chat:

In a followup PR, I'll rename ._parent to ._root_session to avoid confusion.

@wch: That is a better name 👍

Merging

@schloerke schloerke merged commit 381e851 into main Mar 17, 2025
63 of 64 checks passed
@schloerke schloerke deleted the revert_session_parent branch March 17, 2025 17:42
schloerke added a commit that referenced this pull request Mar 17, 2025
@schloerke
Merge branch 'main' into bookmark_modal
c11a16c 
* main:
  fix: Revert - fix(session): Always have calling session be the parent session when making scoped session (#1924)
  fix(session): Always have calling session be the parent session when making a scoped session (#1923)
schloerke added a commit that referenced this pull request Mar 17, 2025
@schloerke
Rename ._parent to ._root_session in SessionProxy
e4d78ec 
Related: #1924 (comment)
@schloerke schloerke mentioned this pull request Mar 17, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@jcheng5 jcheng5 Awaiting requested review from jcheng5

@wch wch Awaiting requested review from wch

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@schloerke @jcheng5