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.

Sign up for GitHub

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

Jump to bottom

fix communication port duplicate issue #134

Merged
merged 1 commit into from
May 30, 2024
Merged

fix communication port duplicate issue #134

merged 1 commit into from
May 30, 2024

Conversation

hjliu0206
Copy link
Contributor

select comm_port and select kernel 5 port maybe duplicated

@welcome Welcome
Copy link

welcome bot commented May 27, 2024

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@hjliu0206
Copy link
Contributor Author

i raised an request to fix the issue
#133

kevin-bates
kevin-bates reviewed May 28, 2024
View reviewed changes
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.

Hi @hjliu0206 - thank you for the issue and PR - this is greatly appreciated and a great catch!

I just had a couple comments on naming and signatures.

@hjliu0206 hjliu0206 changed the title fix common port duplicate issue fix communication port duplicate issue May 29, 2024
kevin-bates
kevin-bates reviewed May 29, 2024
View reviewed changes
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.

We already use comm_port in other locations. Let's consistently use comm_port throughout. Thanks.

github-advanced-security[bot]
github-advanced-security bot found potential problems May 30, 2024
View reviewed changes
gateway_provisioners/kernel-launchers/shared/scripts/server_listener.py
"""Prepares the socket to which the server will send signal and shutdown requests."""
self.comm_socket = self._select_socket()
sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
sock.bind(("0.0.0.0", comm_port))

Check warning

Code scanning / CodeQL

Binding a socket to all network interfaces Medium

'0.0.0.0' binds a socket to all interfaces.
@kevin-bates kevin-bates added the bug Something isn't working label May 30, 2024
kevin-bates
kevin-bates approved these changes May 30, 2024
View reviewed changes
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.

LGTM - thank you @hjliu0206

@kevin-bates kevin-bates merged commit 700090a into jupyter-server:main May 30, 2024
11 of 19 checks passed
@welcome Welcome
Copy link

welcome bot commented May 30, 2024

Congrats on your first merged pull request in this project! 🎉
congrats
Thank you for contributing, we are very proud of you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevin-bates kevin-bates kevin-bates approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hjliu0206 @kevin-bates