-
Notifications
You must be signed in to change notification settings - Fork 345
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
Solution to the typescript issue (hopefully) #2419
Comments
Thanks for the detailed report and more importantly the painful debugging. I remember having to make the change in common to allow cli based debugger such as Ruby pry to work within a gauge execution. I don't recall offhand the reason gauge-ruby bypassed the use of common, but I think it's a safe change to make since AFAIK no other plugin uses stdin the way Ruby does. Look forward to the results of your further changes. Thanks again! |
@bockstaller thanks a lot for the detailed investigation 🙌 |
🫶 |
@bockstaller, this report made my day :) |
Thank you very much for the great response!
|
This is related to getgauge/gauge#2419 - removes problematic line - references issue in the code - updates the dependencies in order to get go build and go test working Signed-off by: Lukas Bockstaller <lukas.bockstaller@posteo.de>
This is related to getgauge/gauge#2419 - removes problematic line - references issue in the code - updates the dependencies in order to get go build and go test working Signed-off by: Lukas Bockstaller <lukas.bockstaller@posteo.de> Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de>
This is related to getgauge/gauge#2419 - removes problematic line - references issue in the code - updates the dependencies in order to get go build and go test working Co-authored-by: Lukas Bockstaller <lukas.bockstaller@posteo.de> Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de> Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
This is related to getgauge/gauge#2419 - removes problematic line - references issue in the code - updates the dependencies in order to get go build and go test working Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de> Signed-off-by: Chad Wilson <chadw@thoughtworks.com>
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>
I don't really have my head around all the different combinations that need to be tested here, but not planning to specifically sanity-test anything on Debugging in general for gauge-ruby probably isn't super high priority, as we dropped support for it from the IDE since I didn't know enough to get it working, and there also seems to be some other issue noted here. Personally I don't use it in such an involved/mature way, so it probably needs additional community help if debugging is a priority on ruby side, and made worse by this change somehow. Nevertheless, gauge-ruby |
* Upgrade getgauge/common to solve ts issue 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 Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de> * Tidy Go modules Signed-off-by: Chad Wilson <chadw@thoughtworks.com> * Bump Gauge version Signed-off-by: Chad Wilson <chadw@thoughtworks.com> --------- Signed-off-by: Lukas Bockstaller <lukas.bockstaller@posteo.de> Signed-off-by: Chad Wilson <chadw@thoughtworks.com> Co-authored-by: Chad Wilson <chadw@thoughtworks.com>
This is more of an investigation report than a bug report, so please excuse me for not using the template.
Issue Description
The gauge ecosystem (
gauge
,gauge-vscode
) has its issues 1, 2 and 3 with the inofficialgauge-ts
plugin.The common theme across these issues is these three lines.
Which are repeated in
lsp.log
for each plugin reload.This is only an issue for projects using @BugDiver
gauge-ts
plugin.In previous cases, this behaviour was fixed by tuning the templates
package.json
, which pinned the typescript/ts-node dependencies to old versions.Motivation
The blessed configuration got quite old; maybe it is time to upgrade.
So, I set up a test project with current versions and dug in.
Investigation
Taking a look at the log lines above and this
architecture drawing points towards an issue between
gauge
andgauge-vscode
. It is the only part where the systems communicate viastdio
and usejsonrpc2
.This means that for some reason,
gauge
isn't able to connect to itsstdin
.But this is only the case for
ts
projects.Dead End / Tangent 1 -> Upgrading `gauge-vscode`
So maybe it is a weird interaction between the project under tests `node_modules` and the `gauge-vscode` plugin. So I upgraded the `gauge-vscode` plugin (I'll open a PR), got the tests running, and no luckProjects using the
python
plugin run smoothly.So maybe the plugins are launched differently...
Dead End / Tangent 2 -> Plugin launch method
The `launcher.js` script of the `gauge-ts` plugin is quite complicated in comparison to the straightforward shell script used by `gauge-python`. I wasted a lot of time investigating the minutia of `child_process.spawn`, the stdio blocking/non-blocking behavior of `node.js`, and rewriting `launcher.js` to a shell script,... No luck.So it isn't
gauge-vscode
, and all plugins are launched the same way.It's time to dive into
gauge-ts
and look at its startup procedure.I added print statements to inspect the StaticLoader component and used the tracing options of
grpc-node
to get information about the communication betweengauge
andgauge-vscode.
We can see that the StaticLoader is doing its thing and loads the implementations successfully.
Then we see that
gauge-ts
is starting itsgrpc
server and binding it to port 56139. (ignore the ERROR state of the tracing loglines).gauge
is connecting to thisgrpc
server and reaches a state where it is started andreading on stdin, writing on stdout
.Then we get the already known three error lines:
Dead End / Tangent 3 -> Upgrading `gauge-ts`
I upgraded `gauge-ts` as well (I'll open a PR). Just to be safe... No luckTo recapitulate:
gauge-vscode
specific behaviourgauge-ts
is launched the same way as the other pluginsgauge-ts
is launching without an issue and connects togauge
and is only killed afterward during the teardown ofgauge
after its crash.So let's look at
gauge
...The Smoking Gun
Where everything looks fine until we reach
prepareCommand
with the culprit of the situation
cmd.Stdin = os.Stdin
.Now everything makes sense:
gauge-vscode
startsgauge
and prepares to communicate withgauge
usingstdio
.gauge
starts itselfgauge
launches thegauge-ts
plugin and usesprepareCommand
in the process. Thecmd.Stdin = os.Stdin
statements connect thestdin
ofgauge
, intended for thejsonrpc2
communication tovscode
, to thegauge-ts
process.gauge
is done with its initialization and wants to access itsstdin
to establish the connection tovscode
. But - ruh roh - thisstdin
is already passed along togauge-ts
...But Why
...is
gauge-ts
the only language plugin affected by this behavior.My understanding is that
node.js
is the only runtime that interacts with the stdin file descriptor in a way that makes it not temporarily unavailable for go.How to Solve the Problem
The
cmd.Stdin = os.Stdin
line was added by @sriv togetgauge/common
in response to this issue getgauge/gauge-ruby#11Nevertheless, the issue was solved in this commit getgauge/gauge-ruby@272b743 directly in the
gauge-ruby
repository without relying ongetgauge/common
.Therefore, I think it is possible to remove the
cmd.Stdin = os.Stdin
line without breaking anything.Way Forward
I'll verify the fix tomorrow with a fresh system and will create some PRs afterward across the different repositories.
@zabil, @sriv, @saikrishna321, @NivedhaSenthil, @BugDiver Feedback is more than welcome!
The text was updated successfully, but these errors were encountered: