Skip to content

Reduce REST traffic for server-side folders #1532

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

isc-bsaviano
Copy link
Contributor

This PR was prompted by my investigation into #1523.

  • Only redirect requests for settings.json within .vscode to the server. None of the other files are likely to exist, and this is the only one that we explicitly support. VS Code hammers the file system looking for files within .vscode so this change eliminates a lot of garbage traffic.
  • Don't call GET /api/atelier/ every tie we try to read the root folder.
  • Only call the OpenedDocument UserAction once per workspace folder.

Copy link
Contributor

@gjsjohnmurray gjsjohnmurray left a comment

Choose a reason for hiding this comment

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

I disagree that the .vscode folder support should be restricted to only the settings.json file. There is already value in being able to have a server-folder-specific launch.json file for debugging, .code-snippets file(s) and (to a lesser extent) tasks.json. Some extensions use .vscode/extensions to store stuff intended for sharing.

For this reason the suggested description of the /_vscode webapp at https://docs.intersystems.com/components/csp/docbook/DocBook.UI.Page.cls?KEY=GVSCO_serverflow#GVSCO_serverflow_folderspec is "Storage for VS Code namespace-specific settings, snippets, debug configurations etc."

@gjsjohnmurray
Copy link
Contributor

  • Only call the OpenedDocument UserAction once per workspace folder.

Is this consistent with how Studio behaved?

@isc-bsaviano
Copy link
Contributor Author

Is this consistent with how Studio behaved?

I believe so. I'm pretty sure Studio only "opens" the project when it loads or the project is switched.

I disagree that the .vscode folder support should be restricted to only the settings.json file. There is already value in being able to have a server-folder-specific launch.json file for debugging, .code-snippets file(s) and (to a lesser extent) tasks.json. Some extensions use .vscode/extensions to store stuff intended for sharing.

VS Code probes the file system for these files constantly, and they never exist. This leads to a ton of junk traffic that clogs up debug logs and can pose problems for small user-based licenses if they cause the number of concurrent connections to rise above 25. I don't think that supporting other file types has value, regardless of the traffic impact. I only support doing this for settings.json because I know Deltanji needs it.

@gjsjohnmurray
Copy link
Contributor

If there's a problem with concurrent connections doesn't this indicate we're not providing a session cookie properly in some cases?

What if the first time on a given connection the extension needs to check existence of .vscode/X.y it asks the server, then caches the name if it doesn't exist and thereafter uses the cache to avoid the REST call? If the user subsequently creates .vscode/X.y through VS Code and saves it to the server we'd remove its name from the cache.

We could even do this for settings.json as long as users understand that after one member of the team creates that file for the first time for a shared namespace the other team members would need to reload their VS Code window, or reconnect. Alternatively we could treat settings.json as a special case and always test its existence on the server rather than adding it to the non-existence cache.

@isc-bsaviano
Copy link
Contributor Author

If there's a problem with concurrent connections doesn't this indicate we're not providing a session cookie properly in some cases?

We are properly using session cookies. The issue is that if a logged in user creates more than a certain number of concurrent connections, each of those connections will take out a license unit instead of them all sharing one unit. This can cause license usage to spike. Removing this traffic should help reduce the chances of that happening.

What if the first time on a given connection the extension needs to check existence of .vscode/X.y it asks the server, then caches the name if it doesn't exist and thereafter uses the cache to avoid the REST call? If the user subsequently creates .vscode/X.y through VS Code and saves it to the server we'd remove its name from the cache.

I would rather us make a decision on what we are going to support. I think that users advanced enough to set up the /_vscode web app are going to expect the stored settings to behave like the rest of VS Code settings and update dynamically. Therefore, I think we should allow the constant pinging of settings.json. However, I don't think there's a good reason to support storing any other file. Debug configurations can be put in the .code-workspace, and tasks don't make sense in our server-side environment. I don't see anything in the docs about snippets being stored in the workspace.

@gjsjohnmurray
Copy link
Contributor

I don't see anything in the docs about snippets being stored in the workspace.

Using the Snippets: Configure Snippets... command on a 2-namespace ISFS workspace:

image

Doc reference: https://code.visualstudio.com/docs/editing/userdefinedsnippets#_project-snippet-scope

@gjsjohnmurray
Copy link
Contributor

Debug configurations can be put in the .code-workspace

So can settings, and like settings there are scenarios where a debug config tailored to a specific namespace can be valuable.

@gjsjohnmurray
Copy link
Contributor

As for license starvation, I ran this experiment, starting with no VS Code instances open:

  1. Restart IRIS.
  2. Log in as me on IRIS Portal and go to System Operation> License Usage > Usage by User
  3. Observe 1 connection, 1 web session, 1 unit against my ID.
  4. Open a fresh VS Code and use Server Manager to connect to the server as me (i.e. expand it to see the namespaces subfolder).
  5. Before even expanding the Namespaces subfolder in Server Manager, in IRIS Portal refresh page and observe my ID having 3 connections, 3 web sessions, 1 unit.
  6. Close VS Code, then refresh the Portal page. My counts are still 3, 3, 1
  7. Repeat step 4, then step 5. My counts are now 5, 5, 1.
  8. Another repeat will take them to 7, 7, 1 and so on until we reach 25, 25, 1.
  9. The next attempt will give an "Unavailable" label instead of the "Namespaces" folder, because my IRIS license is a 10-user one so can't trade me up to using 27 units rather than 1.

Can we do anything to release connections when closing VS Code?

@isc-bsaviano
Copy link
Contributor Author

Doc reference: https://code.visualstudio.com/docs/editing/userdefinedsnippets#_project-snippet-scope

Thanks for the correction.

So can settings, and like settings there are scenarios where a debug config tailored to a specific namespace can be valuable.

I just disagree that supporting these other files is valuable. AFAIK there aren't any external tools that depend on files other than settings.json. IMO it's strange architecturally that we persistent VS Code client configuration on the server. The "server-namespace" storage strategy doesn't neatly map to the "workspace folder" VS Code setting concept. I would prefer to not do this except for cases where it's needed, like settings.

Can we do anything to release connections when closing VS Code?

I don't think this is the cause of that issue because it seems to be happening on open and without much user interaction. However, this may be something good for our extensions to do in the deactivate() function.

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.

3 participants