Skip to content

Log out of web sessions when VS Code exits #1540

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

Merged
merged 1 commit into from
Apr 30, 2025

Conversation

isc-bsaviano
Copy link
Contributor

@gjsjohnmurray
Copy link
Contributor

Using the dev VSIX from this PR alongside the one from my Server Manager PR is doing a much better job of closing down web sessions when exiting, as well as when closing or reloading workspaces (server-side in my use cases). The one lingering /api/atelier session I am seeing may belong to Language Server, and I'm hoping your upcoming change there will tidy this one up.

I think there's perhaps an edge case which your current deactivate strategy doesn't handle.

  1. Create a new workspace to access a namespace on server A.
  2. Add a second folder to it which accesses a namespace on server B.
  3. Remove that folder from the workspace.
  4. Close the workspace.

I think a web session onto B will remain.

Also, when looking at this issue I tried to refresh myself about how this extension has historically stored session cookies in its extensionContext.globalState under "API:host:port" keys, though doing this via the Cache class from the vscode-cache package.

The new code for ending sessions during deactivate almost certainly has consequences for that mechanism, which I assume was intended to avoid re-prompting for credentials, and to share license resources.

However the "API:host:port" key doesn't handle the increasingly common scenarios where host:port serves multiple IRIS instances and pathPrefix is involved.

I don't think these globalState entries ever get deleted. Nor is it possible to do that during deactivate owing to how most of the extension API servicing framework has already been terminated by then.

IMO none of the above needs delay this PR, but I wanted to record the info before I forget it.

@isc-bsaviano
Copy link
Contributor Author

I think there's perhaps an edge case which your current deactivate strategy doesn't handle.

  1. Create a new workspace to access a namespace on server A.
  2. Add a second folder to it which accesses a namespace on server B.
  3. Remove that folder from the workspace.
  4. Close the workspace.

I think a web session onto B will remain.

Yeah, this is an edge case. IMO this logout behavior is a "nice to have" so I'm not worried about it being suboptimal.

Also, when looking at this issue I tried to refresh myself about how this extension has historically stored session cookies in its extensionContext.globalState under "API:host:port" keys, though doing this via the Cache class from the vscode-cache package.

The new code for ending sessions during deactivate almost certainly has consequences for that mechanism, which I assume was intended to avoid re-prompting for credentials, and to share license resources.

However the "API:host:port" key doesn't handle the increasingly common scenarios where host:port serves multiple IRIS instances and pathPrefix is involved.

I don't think these globalState entries ever get deleted. Nor is it possible to do that during deactivate owing to how most of the extension API servicing framework has already been terminated by then.

This code is very old, so it's not surprising that it doesn't take pathPrefix into account. It would be nice to do a review and cleanup of this in a separate PR.

@isc-bsaviano isc-bsaviano marked this pull request as ready for review April 29, 2025 20:23
@isc-bsaviano isc-bsaviano requested a review from isc-rsingh April 29, 2025 20:23
@isc-bsaviano isc-bsaviano merged commit 9d292ea into intersystems-community:master Apr 30, 2025
5 checks passed
@isc-bsaviano isc-bsaviano deleted the logout branch April 30, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants