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

Improve handling of debugging WebSocket messages #1258

Merged

Conversation

isc-bsaviano
Copy link
Contributor

The server generally tries to send one "line" of console output per WebSocket "packet" when debugging. However, in cases where an extreme amount of output is written to the console in a short amount of time , the server may send a 32K chunk of many "lines". Currently our debug adapter chokes on that huge packet and throws an error, and the debugger becomes out of sync with the server. This PR fixes that.

gjsjohnmurray
gjsjohnmurray previously approved these changes Oct 26, 2023
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 though not actually tested

@jpa95
Copy link

jpa95 commented Oct 26, 2023

This looks a lot like the issue we had with #1170. The fix provided at that time (#1174) did help some but not completely, we still have large output tests freeze up (the test goes through but the output window freezes at some point). I juste never go around to re-submitting.

@isc-bsaviano
Copy link
Contributor Author

@gjsjohnmurray @jpa95 @GendronAC I fixed the build error so you can now download this vsix to test the change if you want.

@gjsjohnmurray gjsjohnmurray self-requested a review October 26, 2023 15:03
@GendronAC
Copy link

I installed it and ran a few hundreds testcase without issue :) #HappyDance :shipit:

@isc-bsaviano isc-bsaviano merged commit 9809670 into intersystems-community:master Oct 26, 2023
7 checks passed
@isc-bsaviano isc-bsaviano deleted the debug-message branch October 26, 2023 16:39
@jpa95
Copy link

jpa95 commented Oct 26, 2023

Working great on my specific use case as well. Thanks a lot!

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.

4 participants