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

Notify ChannelQueue that the response router thread is finishing #896

Conversation

CiprianAnton
Copy link
Contributor

In case the response_router encounters an error, the thread will exit and there is no code to check if it's still alive. This would mean waiting for messages that never arrives.

Added a simple flag based notification mechanism where the route_responses will notify channels that the thread is exiting.

Also because the route_responses thread depends on channels to be created, it is important to make sure we don't start this thread before starting channels.

@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2022

Codecov Report

Merging #896 (a717f84) into main (4f1e09e) will increase coverage by 0.92%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
+ Coverage   70.52%   71.44%   +0.92%     
==========================================
  Files          65       65              
  Lines        7698     7705       +7     
  Branches     1287     1289       +2     
==========================================
+ Hits         5429     5505      +76     
+ Misses       1879     1805      -74     
- Partials      390      395       +5     
Impacted Files Coverage Δ
jupyter_server/gateway/managers.py 81.21% <100.00%> (+17.69%) ⬆️
jupyter_server/services/kernels/kernelmanager.py 78.64% <0.00%> (-1.95%) ⬇️
jupyter_server/gateway/gateway_client.py 78.02% <0.00%> (+2.74%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f1e09e...a717f84. Read the comment docs.

@CiprianAnton CiprianAnton marked this pull request as ready for review June 28, 2022 13:44
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.

@CiprianAnton - these changes look good - thank you. Tested the changes via elyra/papermill/nbclient using a k8s-enabled enterprise gateway.

@blink1073 blink1073 merged commit ee40dbc into jupyter-server:main Jun 29, 2022
@blink1073
Copy link
Contributor

@meeseeksdev please backport to 1.x

@lumberbot-app
Copy link

lumberbot-app bot commented Jun 29, 2022

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 1.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 ee40dbc985e45f5c2699626e0f8f350b7f4989ac
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #896: Notify ChannelQueue that the response router thread is finishing'
  1. Push to a named branch:
git push YOURFORK 1.x:auto-backport-of-pr-896-on-1.x
  1. Create a PR against branch 1.x, I would have named this PR:

"Backport PR #896 on branch 1.x (Notify ChannelQueue that the response router thread is finishing)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

blink1073 pushed a commit to blink1073/jupyter_server that referenced this pull request Jun 29, 2022
blink1073 added a commit that referenced this pull request Jun 29, 2022
… router thread is finishing) (#897)

Co-authored-by: Ciprian Anton <ciprian.anton@ni.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants