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

Upgrade getgauge/common to solve typescript server crash issue #2422

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

bockstaller
Copy link
Contributor

The getgauge/common upgrade solves the issue
where the gauge server crashes whenever a project is loaded that uses the ts plugin.

The cause is described here #2419

This commit closes / permanently fixes issues like
getgauge/gauge-vscode#735 getgauge/gauge-vscode#636 getgauge/gauge-vscode#885

The getgauge/common upgrade solves the issue
where the gauge server crashes whenever a project is loaded
that uses the ts plugin.

The cause is described here getgauge#2419

This commit closes/permanently fixes issues like
getgauge/gauge-vscode#735
getgauge/gauge-vscode#636
getgauge/gauge-vscode#885

Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de>
@bockstaller
Copy link
Contributor Author

Sorry @chadlwilson, I know you are still sanity testing ruby but I couldn't resist opening the PR after that debugging journey 😅

Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
@chadlwilson
Copy link
Contributor

The plugin functional tests for PRs seem to run with the master gauge version. With that in mind, do you need a proper gauge release before you try and get other plugin changes made?

chadlwilson
chadlwilson previously approved these changes Oct 26, 2023
@bockstaller
Copy link
Contributor Author

Getting these changes released is currently everything that I need to unblock us on our product side.
The optional upgrades to gauge-vscode and gauge-ts are more nice to haves I produced on my way and won't require another gauge release

@github-actions
Copy link
Contributor

Benchmark Results

java_simple_parallel.csv

Commit CPU Memory Time ExitCode
77e1343 22% 63584 0:20.81 0
77e1343 22% 63788 0:28.84 0
523c400 20% 65628 0:21.50 0
f0ad99f 19% 65600 0:24.44 0

java_gradle_multithreaded.csv

Commit CPU Memory Time ExitCode
77e1343 7% 103976 0:34.70 0
77e1343 7% 97300 0:28.57 0
523c400 6% 102056 0:32.84 0
f0ad99f 6% 123140 0:46.96 0

java_gradle_parallel.csv

Commit CPU Memory Time ExitCode
77e1343 5% 103268 0:36.28 0
77e1343 5% 122608 0:46.91 0
523c400 5% 125048 0:37.07 0
f0ad99f 6% 109420 0:46.74 0

java_simple_serial.csv

Commit CPU Memory Time ExitCode
77e1343 30% 63672 0:17.63 0
77e1343 32% 61496 0:20.52 0
523c400 33% 63508 0:20.32 0
f0ad99f 31% 63352 0:14.68 0

java_gradle_serial.csv

Commit CPU Memory Time ExitCode
77e1343 6% 97060 0:33.47 0
77e1343 6% 94364 0:40.44 0
523c400 6% 121532 0:45.13 0
f0ad99f 6% 124464 0:37.11 0

java_simple_multithreaded.csv

Commit CPU Memory Time ExitCode
77e1343 21% 65728 0:17.27 0
77e1343 26% 63556 0:13.94 0
523c400 27% 65596 0:13.36 0
f0ad99f 26% 63576 0:13.86 0

java_maven_parallel.csv

Commit CPU Memory Time ExitCode
77e1343 37% 203860 0:28.83 0
77e1343 35% 179656 0:34.40 0
523c400 33% 182012 0:32.36 0
f0ad99f 38% 200060 0:32.87 0

java_maven_multithreaded.csv

Commit CPU Memory Time ExitCode
77e1343 46% 179388 0:24.30 0
77e1343 44% 181400 0:32.12 0
523c400 49% 186688 0:22.21 0
f0ad99f 38% 191752 0:26.70 0

java_maven_serial.csv

Commit CPU Memory Time ExitCode
77e1343 45% 199672 0:33.59 0
77e1343 53% 198284 0:29.23 0
523c400 51% 205508 0:32.16 0
f0ad99f 45% 202028 0:26.86 0

Notes

  • The results above are generated by running against seed projects in https://github.com/getgauge/gauge-benchmark
  • These results are not persisted, but on merging to master the benchmark will be rerun.
  • These benchmark are run in Github Actions' agents, which are virtualized. Results are not to be taken as actual benchmarks.Rather, these are indicative numbers and make sense for comparison.

See Workflow log for more details.

@chadlwilson chadlwilson linked an issue Oct 26, 2023 that may be closed by this pull request
@gaugebot
Copy link

gaugebot bot commented Oct 26, 2023

@bockstaller Thank you for contributing to gauge. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

@chadlwilson
Copy link
Contributor

OK, not sure I understand the gauge architecture in sufficient detail to intuitively understand why it's optional on the plugin side of things, so will wait for some input from more expert folks :-)

@bockstaller
Copy link
Contributor Author

I'd like to explain and get corrected if I am wrong.

Gauge uses stdio based jsonrpc2 as the communication channel when used as a language server, as it is the case when used with the vscode extension.
jsonrpc2 expects to have access to stdin whenever it starts up.

The patched prepareCommand function is used in gauge to launch the plugins.
This had the effect that the stdin filedescriptor intended for jsonrpc2 communication got passed on to the plugins gauge launched.

The intention was to allow for debugging with gauge-ruby, but was never used as gauge ruby has its own launcher functionality that passes on os.Stdim

This is only problematic in the case of node.js as runtime used for gauge-ts. As it is the only runtime that connects to the stdin on plugin launch.
This happens in a way that prevents jsonrpc2 to use stdin, which leads to the crash of the gauge server.

All other runtimes used for gauge plugins behave in a nicer way and don't lock out jsonrpc2.

Therefore the only change that could be possible would be to configure the node.js runtime to not have this sideeffect, but this isn't possible as far as I am aware off.

@haroon-sheikh
Copy link
Contributor

@bockstaller can you bump the gauge version as described in Contributing document please, we can then merge and release with the common change.

haroon-sheikh
haroon-sheikh previously approved these changes Oct 26, 2023
Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
@chadlwilson chadlwilson dismissed stale reviews from haroon-sheikh and themself via 2bc6265 October 27, 2023 01:25
@chadlwilson
Copy link
Contributor

@haroon-sheikh I've done this on his behalf since I had the PR locally anyway - will shepherd the tests and merge shortly.

@chadlwilson
Copy link
Contributor

Released as 1.5.6 - enjoy (?!)

@sriv
Copy link
Member

sriv commented Oct 27, 2023

OK, not sure I understand the gauge architecture in sufficient detail to intuitively understand why it's optional on the plugin side of things, so will wait for some input from more expert folks :-)

@chadwilson - not sure I understand the question completely. Which optional plugin feature are we discussing here?

@chadlwilson
Copy link
Contributor

@sriv Sorry for that. I just meant that I wasn't clear why fixing the issue only required releasing the main gauge (server) with an updated gauge-common prepareCommand implementation, and why the various plugins didn't also need to be updated to fix the underlying issue.

But I infer now that updating the other plugins to use latest common is optional insofar as it'd confirm that none of them (accidentally) rely on the old StdIn redirect logic that is now removed, since they all may use Execute*Command from common for different purposes.

If we don't update them now, we may discover unintentional regressions later?

@sriv
Copy link
Member

sriv commented Oct 27, 2023

@bockstaller commented their understanding. I just want to say that this analysis is correct. But also wanted to point out that the use of jsonrpc was something we inherited from the lsp/json-rpc specification. AFAIK the initial lsp ecosystem was jsonrpc over stdio (or maybe we missed the other communication modes). But I do see support for pipes and socket in the [current LSP spec] (https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#implementationConsiderations)

I reckon this is a potential change that can be done at gauge's end.

@chadlwilson
Copy link
Contributor

OK, yes, I read his reply, which is why I merged and released :-)

Just to be clear - do you have any further concern with the impact of this change?

Or particular plugins I can help mitigate risk on by actively updating them to latest gauge-common lib version? Only done gauge-ruby of the go-based plugins thus far.

@sriv
Copy link
Member

sriv commented Oct 27, 2023

It's been a while, but if I believe my memory this specific change was done only for debug support in Ruby and that too for pry like cli debugging and not IDE debugging which works differently.

So, I am going to bet that this won't have an impact on other plugins.

@chadlwilson
Copy link
Contributor

chadlwilson commented Oct 27, 2023

Your memory is certainly correct as to the initial motivation so fingers crossed nothing else subsequently relies on those stdin semantics.

getgauge/common@16d1f84
getgauge/gauge-ruby@272b743

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Solution to the typescript issue (hopefully)
4 participants