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: Pip servers handle config changes gracefully #142

Merged
merged 2 commits into from
Sep 26, 2024

Conversation

bmingles
Copy link
Collaborator

@bmingles bmingles commented Sep 23, 2024

  • Pip servers now support a larger range of ports
  • Configured servers now reserve ports so pip servers won't touch them
  • If configuration changes such that a running pip server becomes reserved by config, the pip server will be shut down
  • Keep server state for running pip servers on config change so that generated PSKs can continue to be used

Testing
I used a configuration targetting the first few ports starting at 10000

Initial config (servers all commented out)

"deephaven.coreServers": [
  // "http://localhost:10000/",
  // "http://localhost:10001/",
  // "http://localhost:10002/",

Pip server port becomes reserved via config change

  • Start a pip server (should start on port 10000)
  • Modify config by uncommenting "http://localhost:10000/"
  • Pip server should automatically stop. localhost:10000 should be listed under "Stopped" and no longer under "Managed"

Start server outside of extension

  • Start a server on port 10000 (outside of extension)
  • Refreshing server panel should show it as running and allow connection

Arbitrary config change

  • Start a pip server
  • Make a config change that doesn't affect the port of the running server
  • Pip server should remain connected and usable

resolves #137

Copy link

End-to-end Test Summary

Tests 📝Passed ✅Failed ❌Skipped ⏭️Pending ⏳Other ❓Flaky 🍂Duration ⏱️
660000004:59:49
A ctrf plugin

Detailed Test Results

NameStatusmsFlaky 🍂
should default to the correct settingspassed ✅1735
should return custom settings: Empty configspassed ✅293
should return custom settings: Populated configspassed ✅191
should be able to load VSCodepassed ✅1038
should only be visible when a supported file type is active: test.groovypassed ✅2783
should only be visible when a supported file type is active: test.pypassed ✅1012
A ctrf plugin

Failed Test Summary

No failed tests ✨

Flaky Test Summary

No flaky tests detected. ✨

@bmingles bmingles requested a review from mofojed September 23, 2024 16:28
* @returns A port number or `null` if no ports are available.
*/
getNextAvailablePort = (): Port | null => {
for (let i = 10000; i < 10050; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

Why stop at 10050? Why not like 10999?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was arbitrary. Figured 50 was even higher than system resources would probably allow

@bmingles bmingles merged commit c39212c into main Sep 26, 2024
4 checks passed
@bmingles bmingles deleted the 137-pip-server-port-conflicts branch September 26, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pip server conflicts with configured servers
2 participants