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

Fix #15: debugging session closes silently #23

Merged
merged 5 commits into from Nov 17, 2021
Merged

Fix #15: debugging session closes silently #23

merged 5 commits into from Nov 17, 2021

Conversation

noppej
Copy link
Contributor

@noppej noppej commented Nov 17, 2021

This fix for #15 includes the following:

  • In addition to checking for error code, also check for the possibility of a process signal.
  • Previously the probe-rs-debugger process was started with child_process.execFile, which has a default maxBuffer for stdio of 1024 * 1024. When the stdout or stderr (RUST_LOG output is handled this way) exceeded this, VSCode would terminate the process and truncate the output ... and because we didn't test the signal, we never saw the SIGTERM event happening, and it looked as if the probe-rs-debugger failed silently. I have changed this to use child_process.spawn, which uses Stream objects instead of Buffer objects to handle the stdio from the child process.
  • Bump the version number of the extension to 0.3.3

@noppej
Copy link
Contributor Author

noppej commented Nov 17, 2021

I am waiting for the original reporter of this problem to test that this fix closes #15.

@noppej noppej linked an issue Nov 17, 2021 that may be closed by this pull request
Comment on lines +300 to +302
if (debuggerStatus === (DebuggerStatus.running as DebuggerStatus)) {
await new Promise<void>((resolve) => setTimeout(resolve, 500)); // Wait for a fraction of a second more, to allow TCP/IP port to initialize in probe-rs-debugger
}
Copy link
Member

Choose a reason for hiding this comment

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

Not directly related to this PR, but what is the reason to use a TCP connection by default?

It seems easier to me to just use stdout to communicate, which requires less setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how I originally had it. I discovered it became unstable (mostly lost requests/responses), which were not reproducible over TCP.

Once I made the changeover, I also found it opened the opportunity to pipe RUST_LOG output to the user's VSCode console, so I figured it was worth the extra effort at extension development time.

What do you mean "requires less setup"?

Copy link
Member

Choose a reason for hiding this comment

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

With stdout, I would assume that that we don't need to wait for any setup, and it would also not be required to find a suitable port to communicate.

The logs are sent to stderr AFAIk, so catching them should still be possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, like I said, that is how I had it working. I implemented the TCP by default based on some posts I found where other extension developers saw similar instability with the STDIO version.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit surprised that STDIO is unstable, but have no reason to doubt it :)

Could you add a comment to explain this? Just to avoid similar questions in the future. Thanks!

@noppej
Copy link
Contributor Author

noppej commented Nov 17, 2021

I am waiting for the original reporter of this problem to test that this fix closes #15.

The user confirmed that this fix now reports the errors correctly, and resolves the "silent closure" reported in #15

@noppej noppej requested a review from Tiwalun November 17, 2021 17:59
src/extension.ts Outdated Show resolved Hide resolved
Comment on lines +300 to +302
if (debuggerStatus === (DebuggerStatus.running as DebuggerStatus)) {
await new Promise<void>((resolve) => setTimeout(resolve, 500)); // Wait for a fraction of a second more, to allow TCP/IP port to initialize in probe-rs-debugger
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit surprised that STDIO is unstable, but have no reason to doubt it :)

Could you add a comment to explain this? Just to avoid similar questions in the future. Thanks!

@noppej noppej requested a review from Tiwalun November 17, 2021 19:40
Copy link
Member

@Tiwalun Tiwalun left a comment

Choose a reason for hiding this comment

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

Thanks!

@Tiwalun
Copy link
Member

Tiwalun commented Nov 17, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Nov 17, 2021

@bors bors bot merged commit 31b1843 into master Nov 17, 2021
@bors bors bot deleted the issue15 branch November 17, 2021 19:58
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.

debugging session closes silently
2 participants