close
Skip to content

Close all unused ports command#244245

Merged
alexr00 merged 6 commits into
microsoft:mainfrom
nojaf:close-all-unused-ports
May 8, 2025
Merged

Close all unused ports command#244245
alexr00 merged 6 commits into
microsoft:mainfrom
nojaf:close-all-unused-ports

Conversation

@nojaf

@nojaf nojaf commented Mar 21, 2025

Copy link
Copy Markdown
Contributor

Hello,

When connecting to a remote dev container, we often encounter the issue where forwarded ports remain open even after the remote process has stopped running. This occurs due to our setup, and VSCode cannot always detect when this happens.

image

We would like to propose adding a command that automatically clears all forwarded ports without an active process.

Would this feature be accepted?

If so, how can I test this locally? I attempted to install a local VSIX of the remote container extension, but I'm unable to start a dev container. I believe this issue may stem from proprietary boundaries. I’m open to any suggestions on how to test this in another way.

@alexr00

alexr00 commented Mar 24, 2025

Copy link
Copy Markdown
Member

You can test it with the test resolver:

  1. build
  2. run Code OSS
  3. from Code OSS you can run the command "Remote-TestResolver: Connect to TestResolver in Current Window"

id: RemoteTunnelCommandIds.closeUnusedPorts,
title: RemoteTunnelCommandLabels.closeUnusedPorts,
category: REMOTE_TUNNEL_CATEGORY,
menu: []

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When the remote system is NOT linux or if remote.autoForwardPortsSource is set to output, then none of the ports have a running process.

When the remote system is linux and the setting remote.autoForwardPortsSource is not set to output, I think this command makes sense. I would add the command to the command palette in this case.

@nojaf

nojaf commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

3. Remote-TestResolver

Thank you for this pointer @alexr00!
I'm not quite seeing my command after entering "Remote-TestResolver: Connect to TestResolver in Current Window".
At first I thought I was missing menu: [{ id: MenuId.CommandPalette }] in my own code but that doesn't seem to be it.

Am I expected to add anything to vscode-test-resolver/src/extension.ts before I can test my new command?

@alexr00

alexr00 commented Mar 24, 2025

Copy link
Copy Markdown
Member

Hmm, that should be it. You should just be able to copy the precondition and menu from lines 701 and 702.

@nojaf

nojaf commented Mar 24, 2025

Copy link
Copy Markdown
Contributor Author

Hmm, no that isn't working out for me.
Is my code registered in the right place?
It seemed like a good place, but maybe the electron-sandbox bit is never loaded?

@nojaf

nojaf commented Mar 28, 2025

Copy link
Copy Markdown
Contributor Author

Hi @alexr00, any thoughts here?

@nojaf

nojaf commented Apr 2, 2025

Copy link
Copy Markdown
Contributor Author

Hello @alexr00,

I was able to get this working locally:
Kapture 2025-04-02 at 17 00 06

I needed to add

"tunnelApplicationConfig": {
		"authenticationProviders": {
		  "github": {
			"scopes": [
			  "repo",
			  "user:email"
			]
		  }
		},
		"editorWebUrl": "https://vscode.dev",
		"extension": {
		  "extensionId": "ms-vscode.remote-server",
		  "friendlyName": "Remote Server"
		}
	  },

to product.json in order for the registration to come through.

Any thoughts on how I can test the conditions part?
What would I need and how to emulate this?

@alexr00 alexr00 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR! Sorry about the bad instructions for testing locally, I thought you were working on a command for all remotes, not just for the Remote Tunnels.

I actually think a "close all ports which don't have a running process" is a useful command for any remote, not just remote tunnels. Would you be willing to see if you could move the action into https://github.com/nojaf/vscode/blob/main/src/vs/workbench/contrib/remote/electron-sandbox/remote.contribution.ts ?

Then, in your command precondition you would also need to check if the remote is Windows, as Windows remotes will never show as having an associated process.

@nojaf nojaf force-pushed the close-all-unused-ports branch from 97ea615 to 538f756 Compare May 6, 2025 08:44
@nojaf

nojaf commented May 6, 2025

Copy link
Copy Markdown
Contributor Author

Hi @alexr00, thank you for the feedback!
I tried to address this, let me know what you think!

menu: [{
id: MenuId.CommandPalette
}],
precondition: ContextKeyExpr.and(ContextKeyExpr.notEquals(`config.${PORT_AUTO_SOURCE_SETTING}`, PORT_AUTO_SOURCE_SETTING_OUTPUT), RemoteNameContext)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The contexts you were using @nojaf weren't using the OS of the remote, they were using the local OS. I've looked around but we don't seem to already have broadly useful remote OS context. I removed them, and added a context for whether we're connected to a remote.

I've inlined your strings and also used constants where we have them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for these changes!

@alexr00 alexr00 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for making the changes! I made a few of my own, and I have one question.

// collect all forwarded ports and filter out those who do not have a process running
const forwarded = remoteExplorerService.tunnelModel.forwarded;
for (const [_, tunnel] of forwarded) {
if (!tunnel.hasRunningProcess) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think if you check tunnel.hasRunningProcess === false here instead of checking for undefined then you'll only close tunnels for which we don't have a running process, not simple ones which we don't know about (like on Windows). Can you try it out and see if that's the case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. It still worked for me after changing this.

@alexr00 alexr00 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, thank you!

@alexr00 alexr00 enabled auto-merge (squash) May 8, 2025 08:57
@vs-code-engineering vs-code-engineering Bot added this to the May 2025 milestone May 8, 2025
@alexr00 alexr00 merged commit 714d186 into microsoft:main May 8, 2025
7 checks passed
@vs-code-engineering vs-code-engineering Bot locked and limited conversation to collaborators Jun 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants