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

Client-side improvements #1470

Merged

Conversation

isc-bsaviano
Copy link
Contributor

This PR addresses feedback from a DC beta tester.

  • Limit concurrent file system reads on initial folder indexing to 250 to avoid EMFILE errors.
  • Limit concurrent file update REST requests to 50 to avoid overloading the server.
  • Always get the text of a document from the file system, even if VS Code has it loaded, to avoid concurrency issues when the file system updates before VS Code has updated its version.
  • Avoid unneeded file system reads after export by updating the document index using the already known text of the file that was exported.
  • New rate limiting utility code that is better documented

gjsjohnmurray
gjsjohnmurray previously approved these changes Jan 31, 2025
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.

LGTM but I haven't tested it.

@isc-bsaviano
Copy link
Contributor Author

Thanks John. I will give the issue reporter at least a few days to respond.

@TwaHendrik
Copy link

Hi @isc-bsaviano,

Thank you for your quick changes.
We tested a lot.

Deleting and discard:

  • Delete File in VS Code (Works)
  • File in VS Code discard (Works)
  • File in VS Code modified (Works) and discard (Works)
  • Multiple Files in VS Code modified and discard (Works)
  • File via external GitClient-Tool deleted/modified. Sync to VS Code and IRIS (Works)

Adding and discard:

  • add new File (with "objectscript.autoAdjustName": true) (Works)
  • add new File and discard (Works)
  • Git pull with deleted, modified and added files (Works)
  • Git switch from main to branch with deleted, modified and added files (Works)
  • Add new File while iris is down
    • After iris has started again, File is not created in IRIS. It was tried with an error. TypeError: Cannot read properties of null (reading 'with')

Modified:

  • File modified and discard (Works)

Findings:
IRIS Compiles the classes multiple times (min. two times, sometimes more than 4 times). Sometimes iris throw an compile error. Seems to be a race condition. Maybe because the multi compile for the same classes is executed by different processes.

Compilation started on 02/03/2025 14:36:54 with qualifiers 'cukb /checkuptodate=expandedonly'
ERROR #5864: User 'CSP Gateway' in process '7916' has 'sample.Sample.cls' open for editing.
Detected 1 errors during compilation in 0.074s.
 
Compilation started on 02/03/2025 14:36:54 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling table sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.315s.

Some feature requests:
New file status for uncompiled classes. Similar to the old studio '+'. It shows that the classes could no compiled but saved.

@isc-bsaviano
Copy link
Contributor Author

@TwaHendrik Thank you for testing this! I'm glad that most of your issues have been resolved.

Add new File while iris is down. After iris has started again, File is not created in IRIS. It was tried with an error. TypeError: Cannot read properties of null (reading 'with')

I will try to reproduce this.

IRIS Compiles the classes multiple times (min. two times, sometimes more than 4 times). Sometimes iris throw an compile error. Seems to be a race condition. Maybe because the multi compile for the same classes is executed by different processes.

The classes should only be compiled once, so if they are being compiled more than once then there's a bug. The compilation works by batching as many documents into one request as possible, until there's been 500ms with no activity. The the batch of documents is sent to the server to compile. The only exception is if the document in the active text editor is being compiled. In that case, that document and any others in the batch are compiled right away to avoid any perceived latency when a user saves the document they are working on. I might have to remove that special case and increase the delay so more batching occurs.

New file status for uncompiled classes. Similar to the old studio '+'. It shows that the classes could no compiled but saved.

This has been considered in the past. I haven't done it because I'd have to ping the server a lot to make sure that decoration is accurate.

@isc-bsaviano
Copy link
Contributor Author

Add new File while iris is down. After iris has started again, File is not created in IRIS. It was tried with an error. TypeError: Cannot read properties of null (reading 'with')

I tried to reproduce this but I'm not sure what to do. If a file is created outside of VS Code then we won't try to sync it when VS Code starts up. Files are only synced when they are created/changed/deleted while VS Code is running. If VS Code is running but the server connection is inactive, VS Code will not attempt the sync.

@TwaHendrik
Copy link

@isc-bsaviano #

Add new File while iris is down. After iris has started again, File is not created in IRIS. It was tried with an error. TypeError: Cannot read properties of null (reading 'with')

I tried to reproduce this but I'm not sure what to do. If a file is created outside of VS Code then we won't try to sync it when VS Code starts up. Files are only synced when they are created/changed/deleted while VS Code is running. If VS Code is running but the server connection is inactive, VS Code will not attempt the sync.

We have shut down the IRIS instance and thus deactivated the connection. Then we edited a file in VS Code and thus triggered an incositence between the client-side workspace and IRIS. Then we started IRIS again and VS Code tried to transfer the document to IRIS. The error message was then thrown.

We have surmised that the extension is trying to retransmit the document after the connection has been activated again in order to resolve the inconsistency. If this function is not available, the point can be ignored.

@isc-bsaviano
Copy link
Contributor Author

We have shut down the IRIS instance and thus deactivated the connection. Then we edited a file in VS Code and thus triggered an incositence between the client-side workspace and IRIS. Then we started IRIS again and VS Code tried to transfer the document to IRIS. The error message was then thrown.

I just checked and the extension is not doing this automatically. If the server connection is inactive at the time the change was made, the change is not stored for later. Re-activating a server connection will not prompt a sync with the server. If you manually triggered a sync (by editing and saving the file or importing it) then I will try to replicate exactly what you did so I can see that error.

@isc-bsaviano
Copy link
Contributor Author

Hi @TwaHendrik, can you test this vsix and let me know if it fixes your multiple-compile/locking race condition issue?
vscode-objectscript-2.12.11-dev.1470.vsix.zip

@TwaHendrik
Copy link

Hi @TwaHendrik, can you test this vsix and let me know if it fixes your multiple-compile/locking race condition issue? vscode-objectscript-2.12.11-dev.1470.vsix.zip

Hi @isc-bsaviano, unfortunately the error is still there. We have saved only one class in vs code via ctrl + s and the class has been compiled 10 times. The output is:

Compilation started on 02/06/2025 13:04:48 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.372s.
 
Compilation started on 02/06/2025 13:04:49 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.364s.
 
Compilation started on 02/06/2025 13:04:50 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.360s.
 
Compilation started on 02/06/2025 13:04:50 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.382s.
 
Compilation started on 02/06/2025 13:04:51 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.340s.
 
Compilation started on 02/06/2025 13:04:52 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.404s.
 
Compilation started on 02/06/2025 13:04:53 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.345s.
 
Compilation started on 02/06/2025 13:04:54 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.364s.
 
Compilation started on 02/06/2025 13:04:54 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class sample.Sample
Compiling routine sample.Sample.1
Compilation finished successfully in 0.356s.

For the error message

ERROR #5864: User 'CSP Gateway' in process '7916' has 'sample.Sample.cls' open for editing.
Detected 1 errors during compilation in 0.074s.

We have investigated the occurrence of the error message. We use a class extends %Studio.Extension.Base with OnBeforeCompile. This performs a static code analysis with every compile. This ensures a longer compilation time, which increases the probability of conflicts occurring during multiple compilation. However, we could not trigger Error 5864 without our %Studio.Extension.Base. It could be an indication that the possibility of this error is higher for large classes that take longer to compile.

However, the problem of multiple compilation (without the error 5864) is independent of %Studio.Extension.Base.

@isc-bsaviano
Copy link
Contributor Author

Thank you for testing again, and for the extra details. I'm still not able to replicate the multi-compile. Here's what I did:

  1. Changed my compile flags to match yours.
  2. Created a Projection class that hangs for 1 second on compilation to simulate your slow-compiling classes.
  3. Added this projection to a class definition I have in my test client-side folder.
  4. Pressed Ctrl-S.
  5. The class saved and then compiled once, which took ~1.02 seconds.

Do you only see that 5864 message in the compiler output, or does it occur as a response to another request as well? For example, we make a GET /doc/{document} request after compilation is complete to refresh the contents of the file in VS Code. If it only happens during compilation, then I think it's another symptom of the same core issue that classes should only be compiling once.

@TwaHendrik
Copy link

Hi @isc-bsaviano ,

we perform a very simple test.
We do the following in the namespace %SYS without projection.

Create a new class and save via ctrl + s

Class %ZLib.Dummy Extends %RegisteredObject
{
 
Property vv;
 
}

Compile 3 times.
Output

Compilation started on 02/06/2025 14:59:02 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class %ZLib.Dummy
Compiling routine %ZLib.Dummy.1
Compilation finished successfully in 0.034s.
 
Compilation started on 02/06/2025 14:59:03 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class %ZLib.Dummy
Compiling routine %ZLib.Dummy.1
Compilation finished successfully in 0.012s.
 
Compilation started on 02/06/2025 14:59:04 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class %ZLib.Dummy
Compiling routine %ZLib.Dummy.1
Compilation finished successfully in 0.015s.

We remove the property and save via ctrl + s


Class %ZLib.Dummy
{

}

Compile 2 times.
Output:

Compilation started on 02/06/2025 15:00:01 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class %ZLib.Dummy
Compilation finished successfully in 0.027s.
 
Compilation started on 02/06/2025 15:00:01 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class %ZLib.Dummy
Compilation finished successfully in 0.004s.

Add a new ClassMethod and save via ctrl + s

Class %ZLib.Dummy
{
 
ClassMethod aa()
{
}
 
}

Compiling error
Output:

ERROR #5864: User 'CSP Gateway' in process '12804' has '%ZLib.Dummy.cls' open for editing.
 
Compilation started on 02/06/2025 15:00:58 with qualifiers 'cukb /checkuptodate=expandedonly'
Compiling class %ZLib.Dummy
Compiling routine %ZLib.Dummy.1
Compilation finished successfully in 0.030s.

We perfom the test on an empty iris instance.

@isc-bsaviano
Copy link
Contributor Author

I have not heard from Hendrick in a while and have yet to reproduce his issue. I'd like to get this in since it's still an improvement. I can implement a fix for his issue in a new PR if it can be reproduced.

@isc-bsaviano isc-bsaviano merged commit e9dba8c into intersystems-community:master Feb 20, 2025
5 checks passed
@isc-bsaviano isc-bsaviano deleted the fix-client-side branch February 20, 2025 12:00
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