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

Fix to remove potential memory leak on Jupyter Notebooks ZMQChannelHandler code #6251

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

Vishwajeet0510
Copy link
Contributor

Adding a session to the _open_sessions list only after validating kernel. This will prevent memory wastage if any requests are made with an incorrect kernel ID.

@Vishwajeet0510 Vishwajeet0510 changed the title Fix to add session only after kernel validation Fix to remove potential memory leak on Jupyter Notebooks ZMQChannelHandler code Dec 24, 2021
Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

@Vishwajeet0510
Copy link
Contributor Author

Hey @kevin-bates

Do you have any idea when this change will be released?

@kevin-bates kevin-bates merged commit 479902d into jupyter:master Jan 25, 2022
@kevin-bates
Copy link
Member

kevin-bates commented Jan 25, 2022

Sorry for the delay @Vishwajeet0510! I'll try to cut a 6.4.8 release soon (within the next day or two).

@kevin-bates
Copy link
Member

FYI - Notebook 6.4.8 is available on pypi: https://pypi.org/project/notebook/6.4.8/. Thanks for your help @Vishwajeet0510 and @rahul26goyal.

@blink1073
Copy link
Member

I added this to jupyter-server/jupyter_server#110, if you'd like to make the same submission in the new Jupyter Server @Vishwajeet0510

@kevin-bates
Copy link
Member

Ah, yes, thank you Steve!

@Vishwajeet0510 - since EG uses JupyterServer in more recent releases, a similar change would be advised. Thank you!

@Vishwajeet0510
Copy link
Contributor Author

Hello @kevin-bates and @blink1073

Thanks for the quick followup and release. I will create a similar PR for jupyter_server as well.

@rahul26goyal
Copy link

thanks a lot @kevin-bates for all the help in resolving this issue. :)

@Vishwajeet0510
Copy link
Contributor Author

Hello @kevin-bates

Is it possible to back port this fix so it is available with Notebook v6.0.3? We are currently using JEG v2.1.0 and it is not compatible with Jupyter Notebook version >6.0.3. Please let us know if this would be possible.

@kevin-bates
Copy link
Member

Hi @Vishwajeet0510 - could you please elaborate on why EG 2.1.0 is incompatible with NB >6.0.3? The only compatibility issue I'm aware of is jupyter_client > 7 and EG has caps in place (in its more recent releases) to address that.

@Vishwajeet0510
Copy link
Contributor Author

Hi @kevin-bates

As per my understanding, from Jupyter Notebook v6.1.0 async kernel management is supported and corresponding changes on JEG side are supported from JEG v2.1.1 onwards.

@kevin-bates
Copy link
Member

Hi @Vishwajeet0510 - since this is EG-specific and I'd like to explore things (possibly creating a longer comment chain), I've opened this discussion item in EG and hoping you and @rahul26goyal can join me there: jupyter-server/enterprise_gateway#1037

We can update this thread once things are resolved. Thank you.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants