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 port selection #1229

Merged
merged 7 commits into from
Mar 6, 2023
Merged

Conversation

blink1073
Copy link
Contributor

Fixes #1170, by using the same bind_sockets function as is later used to start the http server.

@blink1073 blink1073 added the bug label Mar 6, 2023
@blink1073
Copy link
Contributor Author

@kevin-bates should we do something similar in jupyter_client?

@blink1073
Copy link
Contributor Author

The tornado util function appears to be more robust.

@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (b5a6306) 79.08% compared to head (6335a8f) 79.12%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1229      +/-   ##
==========================================
+ Coverage   79.08%   79.12%   +0.03%     
==========================================
  Files          68       68              
  Lines        8253     8252       -1     
  Branches     1603     1599       -4     
==========================================
+ Hits         6527     6529       +2     
+ Misses       1301     1299       -2     
+ Partials      425      424       -1     
Impacted Files Coverage Δ
jupyter_server/serverapp.py 72.76% <100.00%> (+0.06%) ⬆️
jupyter_server/services/contents/filemanager.py 76.31% <0.00%> (-0.62%) ⬇️
...ter_server/services/kernels/connection/channels.py 61.72% <0.00%> (+1.32%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kevin-bates
Copy link
Member

@kevin-bates should we do something similar in jupyter_client?

I think this makes sense. Are we confident the SO_LINGER portion is unnecessary? This also more closely binds us (pun intended) to tornado - although its exclusion would likely occur in an entirely different code base - but wanted to mention that as a possible deterrent.

@blink1073
Copy link
Contributor Author

Yeah, I don't think we need LINGER because we aren't actually sending any messages on this socket. We could always inline the method with attribution if we ever decide to move off of tornado.

@blink1073 blink1073 merged commit 4d311b2 into jupyter-server:main Mar 6, 2023
@blink1073
Copy link
Contributor Author

Actually, I don't think we need this logic in client, since we're asking the OS to give us a port, not attempting to use a specific one.

@blink1073 blink1073 deleted the fix-port-selection branch March 6, 2023 18:00
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.

wrong jupyter server port number in a specific situation
2 participants