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

Session and Kernel object are cloned upon server-side changes #6142

Closed
rajabricks opened this issue Mar 26, 2019 · 7 comments
Closed

Session and Kernel object are cloned upon server-side changes #6142

rajabricks opened this issue Mar 26, 2019 · 7 comments

Comments

@rajabricks
Copy link

@rajabricks rajabricks commented Mar 26, 2019

Update : I found that #3669 is related to what I've posted here and somewhat resolves it, but the 2nd case (session.update()) is still a bug. I've hidden the first case.

Describe the bug
Tl;dr
If a session’s model changes on the backend, the session and/or the session’s kernel will be cloned, but the original session/kernel will remain in memory. This leads to redundant websockets and objects in memory.

Full Issue:
There are two cases where the above can happen:

First case 1. SessionManager refreshes running sessions by fetching session models from the server. If these new models are different from the models it currently has it will update its models and then signal this fact to listeners. The ServiceManager (a listener) will pick up this signal and will in turn attempt to connect to the new session model. The connectTo methods of session and kernel objects will clone the session and its kernel.

  1. When the SessionManager fetches session models from the server, it will update existing session models (if there are differences between the existing and fetched models). If a session is updated, this update method will call connectTo on an existing session’s kernel, resulting in it being cloned.

More details:

First case 1. When the SessionManager fetches session models from the server (via `_refreshRunning()` which is called every ten seconds) and notices that this data is different from its current session models, it emits a signal (`this._runningChanged.emit(models)`).

When the ServiceManager (jupyterlab/packages/help-extension/src/index.tsx) picks up this signal, it attempts to connect to this newly added session.

However, the ConnectTo(model, settings) method of DefaultSession will clone a session if it already exists (i.e. is in runningSessions). This cloned session will in turn attempt to connect to the kernel of the original session. This will clone the kernel. When sessions or kernels are cloned, the original object is not disposed of.

2. SessionManager calls `this._refreshRunning()` every ten seconds. This calls DefaultSession's `listRunning()` which calls `updateRunningSessions()`.
If a newly fetched session model's ID matches the ID of an existing session model, `update()` will be called on the existing session model.

Within update(), the session will attempt to connect to the new model's kernel and as a result, clone it.

The result is that the original kernel object (which is redundant now) will not be disposed of and remain in memory. Furthermore, the original kernel’s web socket will not be closed. This is a bug, but does not seem to noticeably affect performance.

When could this occur?
This could occur very frequently if the user has a server extension that swaps a session's kernel server-side for a new kernel. Even if this new kernel's ID is the same, if any other information about it is different, it will cause cloning.

To Reproduce
Steps to reproduce the behavior for the first case:

  1. Load up JupyterLab
  2. Open Chrome Console
  3. Connect to a kernel ( I used Python3)
  4. observe in the console logs that a new kernel is created with its own websocket. See Figure 2 and 3.

The same thing will occur in the second case, every time a session's model is changed server-side.

Expected behavior
If a session's model changes on the backend, the new session should be used instead, and the previous session (and its kernel) should be disposed of safely.

Screenshots
Stack Trace + cloning of a running session and kernel
Figure 1 - Stack Trace + cloning of a running session and kernel

Console log of a kernel being cloned (and a new websocket being created)
Figure 2 - Console log of a kernel being cloned (and a new websocket being created)

First websocket is unused
Figure 3 - First websocket is unused. Networks Tab -> WS.

Desktop (please complete the following information):

  • OS: macOS High Sierra (10.13.6)
  • Browser: Chrome 73.0.3683.86
  • JupyterLab: 0.35.4

Additional context
Add any other context about the problem here.

If available, please include the following details:

Troubleshoot Output
$PATH:
	/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/bin
	/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/bin
	/Users/raja.shravan/miniconda3/bin
	/Users/raja.shravan/.pyenv/shims
	/Library/Frameworks/Python.framework/Versions/3.5/bin
	/usr/local/bin
	/usr/bin
	/bin
	/usr/sbin
	/sbin

sys.path:
/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/bin
/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/lib/python37.zip
/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/lib/python3.7
/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/lib/python3.7/lib-dynload
/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/lib/python3.7/site-packages

sys.executable:
/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/bin/python3.7

sys.version:
3.7.2 (default, Dec 29 2018, 00:00:04)
[Clang 4.0.1 (tags/RELEASE_401/final)]

platform.platform():
Darwin-17.7.0-x86_64-i386-64bit

which -a jupyter:
/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/bin/jupyter
/Users/raja.shravan/miniconda3/envs/second_3.7.2-env/bin/jupyter
/Users/raja.shravan/.pyenv/shims/jupyter
/usr/local/bin/jupyter

pip list:
Package Version
----------------- ----------
appnope 0.1.0
attrs 19.1.0
backcall 0.1.0
bleach 3.1.0
certifi 2018.11.29
decorator 4.3.2
entrypoints 0.3
ipykernel 5.1.0
ipython 7.3.0
ipython-genutils 0.2.0
jedi 0.13.3
Jinja2 2.10
jsonschema 3.0.1
jupyter-client 5.2.4
jupyter-core 4.4.0
jupyterlab 0.35.4
jupyterlab-server 0.2.0
MarkupSafe 1.1.1
mistune 0.8.4
nb-conda-kernels 2.2.1
nbconvert 5.3.1
nbformat 4.4.0
notebook 5.7.4
pandocfilters 1.4.2
parso 0.3.4
pexpect 4.6.0
pickleshare 0.7.5
pip 19.0.3
prometheus-client 0.6.0
prompt-toolkit 2.0.9
ptyprocess 0.6.0
Pygments 2.3.1
pyrsistent 0.14.11
python-dateutil 2.8.0
pyzmq 18.0.1
Send2Trash 1.5.0
setuptools 40.8.0
six 1.12.0
terminado 0.8.1
testpath 0.4.2
tornado 5.1.1
traitlets 4.3.2
wcwidth 0.1.7
webencodings 0.5.1
wheel 0.33.1

conda list:
# packages in environment at /Users/raja.shravan/miniconda3/envs/second_3.7.2-env:
#
# Name Version Build Channel
appnope 0.1.0 py37_1000 conda-forge
attrs 19.1.0 py_0 conda-forge
backcall 0.1.0 py_0 conda-forge
bleach 3.1.0 py_0 conda-forge
ca-certificates 2018.11.29 ha4d7672_0 conda-forge
certifi 2018.11.29 py37_1000 conda-forge
curl 7.64.0 heae2a1f_2 conda-forge
decorator 4.3.2 py_0 conda-forge
entrypoints 0.3 py37_1000 conda-forge
expat 2.2.5 h0a44026_1002 conda-forge
gettext 0.19.8.1 hcca000d_1001 conda-forge
git 2.21.0 pl526h3e3e3d1_0 conda-forge
ipykernel 5.1.0 py37h24bf2e0_1002 conda-forge
ipython 7.3.0 py37h24bf2e0_0 conda-forge
ipython_genutils 0.2.0 py_1 conda-forge
jedi 0.13.3 py37_0 conda-forge
jinja2 2.10 py_1 conda-forge
jsonschema 3.0.1 py37_0 conda-forge
jupyter_client 5.2.4 py_3 conda-forge
jupyter_core 4.4.0 py_0 conda-forge
jupyterlab 0.35.4 py37_0 conda-forge
jupyterlab_server 0.2.0 py_0 conda-forge
krb5 1.16.3 hcfa6398_1001 conda-forge
libcurl 7.64.0 he376013_2 conda-forge
libcxx 4.0.1 hcfea43d_1
libcxxabi 4.0.1 hcfea43d_1
libedit 3.1.20181209 hb402a30_0
libffi 3.2.1 h475c297_4
libiconv 1.15 h1de35cc_1004 conda-forge
libsodium 1.0.16 h1de35cc_1001 conda-forge
libssh2 1.8.0 hb1dc21d_1004 conda-forge
markupsafe 1.1.1 py37h1de35cc_0 conda-forge
mistune 0.8.4 py37h1de35cc_1000 conda-forge
nb_conda_kernels 2.2.1 py37_0 conda-forge
nbconvert 5.3.1 py_1 conda-forge
nbformat 4.4.0 py_1 conda-forge
ncurses 6.1 h0a44026_1
notebook 5.7.4 py37_1 conda-forge
openssl 1.1.1b h1de35cc_0 conda-forge
pandoc 2.6 1 conda-forge
pandocfilters 1.4.2 py_1 conda-forge
parso 0.3.4 py_0 conda-forge
perl 5.26.2 h79c3c89_1002 conda-forge
pexpect 4.6.0 py37_1000 conda-forge
pickleshare 0.7.5 py37_1000 conda-forge
pip 19.0.3 py37_0
prometheus_client 0.6.0 py_0 conda-forge
prompt_toolkit 2.0.9 py_0 conda-forge
ptyprocess 0.6.0 py37_1000 conda-forge
pygments 2.3.1 py_0 conda-forge
pyrsistent 0.14.11 py37h1de35cc_0 conda-forge
python 3.7.2 haf84260_0
python-dateutil 2.8.0 py_0 conda-forge
pyzmq 18.0.1 py37h4cc6ddd_0 conda-forge
readline 7.0 h1de35cc_5
send2trash 1.5.0 py_0 conda-forge
setuptools 40.8.0 py37_0
six 1.12.0 py37_1000 conda-forge
sqlite 3.26.0 ha441bb4_0
terminado 0.8.1 py37_1001 conda-forge
testpath 0.4.2 py37_1000 conda-forge
tk 8.6.9 ha441bb4_1000 conda-forge
tornado 5.1.1 py37h1de35cc_1000 conda-forge
traitlets 4.3.2 py37_1000 conda-forge
wcwidth 0.1.7 py_1 conda-forge
webencodings 0.5.1 py_1 conda-forge
wheel 0.33.1 py37_0
xz 5.2.4 h1de35cc_4
zeromq 4.2.5 h0a44026_1006 conda-forge
zlib 1.2.11 h1de35cc_3

Command Line Output

Browser Output

@rajabricks
Copy link
Author

@rajabricks rajabricks commented Mar 26, 2019

@meeseeksdev tag pkg:services

@fcollonval
Copy link
Member

@fcollonval fcollonval commented Mar 31, 2019

Hey @rajabricks
Thanks for this precise description. I think you cornered the source of what I have seen in #5914 (comment) (link for cross-reference).

@jasongrout jasongrout added this to the 1.0 milestone Apr 3, 2019
@jasongrout jasongrout self-assigned this Apr 3, 2019
@rajabricks
Copy link
Author

@rajabricks rajabricks commented Apr 12, 2019

@fcollonval @jasongrout
Sorry for the late reply.
From what I understand, @jasongrout is working on a refactor on how kernels are managed internally. Do you think that refactor would solve this issue? (in which case I could shelf this for now)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 12, 2019

I was, though that work is now slated for post-1.0, as it turned out to be too ambitious for right now. If there is a way to fix this issue in the current codebase, that will still be valuable.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 26, 2019

@rajabricks - since you understand this so well, do you want to go ahead and submit a PR? I think your suggestion of disposing the old session is the first thing I'd try.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented May 26, 2019

I think perhaps it may be enough to just dispose oldValue in

if (this._kernel.isDisposed || model.kernel.id !== this._kernel.id) {
let newValue = Kernel.connectTo(model.kernel, this.serverSettings);
let oldValue = this._kernel;
this.setupKernel(newValue);
this._kernelChanged.emit({ oldValue, newValue });
this._handleModelChange(oldModel);
return;
}

jasongrout added a commit to jasongrout/jupyterlab that referenced this issue Jun 7, 2019
Fixes jupyterlab#6142

Before, a kernel was handed to the session, so it was confusing who was responsible for disposing the kernel connection. Now the API is changed so that the DefaultSession creates the kernel connection, so it is responsible for disposing it.
@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants