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

Update session and kernel manager data only if there was a real change. #9189

Merged
merged 3 commits into from Oct 22, 2020

Conversation

jasongrout
Copy link
Contributor

References

Fixes #9133

Code changes

The rest api returns more information than we expect (according to our typing), like a last activity indicator on a kernel, which always updates. This means that our existing deep compare of the model we have and the model on the server always updates what we have, which triggers a reconnection to the kernel.

An alternative to this approach of hand-coding which attributes is to strip these extra attributes off in the response validation stage, for example in

export function validateModel(data: any): asserts data is Session.IModel {
. The advantage of doing that is that the objects we are passing around are comprehensively typed, and we know what we are getting, and there aren't undocumented attributes that are getting passed around and relied on that are not part of our official api. The disadvantage is that these extra attributes do seem pretty useful, but are not part of the official rest api spec, so perhaps we can let them live with the understanding that they are unofficial.

User-facing changes

Websocket kernel connections are not continually created, as described in #9133

Backwards-incompatible changes

…ed to update.

This was accidentally comparing extra information the server gives us, and in particular the last activity time was *always* forcing an update. Instead, we just check relevant attributes to determine if we need to update.

Fixes jupyterlab#9133
…e is needed.

This solves the same problem as the previous commit, but for kernels.
@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout jasongrout added this to the 3.0 milestone Oct 20, 2020
Copy link
Contributor

@ellisonbg ellisonbg left a comment

Choose a reason for hiding this comment

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

Jason walked me through this code and we discussed. This change resolves the issue and looks good.

@ellisonbg ellisonbg merged commit 1cd38bf into jupyterlab:master Oct 22, 2020
@jasongrout
Copy link
Contributor Author

@meeseeksdev backport to 2.2.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this pull request Oct 22, 2020
ajbozarth added a commit that referenced this pull request Oct 23, 2020
…9-on-2.2.x

Backport PR #9189 on branch 2.2.x (Update session and kernel manager data only if there was a real change.)
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Apr 21, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:services status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reloading a running kernel opens multiple new websockets
2 participants