Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Delay connecting to dlv until it's listening #1354

Merged
merged 1 commit into from
Nov 23, 2017
Merged

Delay connecting to dlv until it's listening #1354

merged 1 commit into from
Nov 23, 2017

Conversation

DMarby
Copy link
Contributor

@DMarby DMarby commented Nov 19, 2017

#473

In a project with external dependencies, debugging hang indefinitely for me as dlv produces output while compiling the go code, before it starts the api server.
This caused VS Code to attempt to connect to dlv the api server was listening.

In order to avoid this, and to avoid arbitrary delays in the code, this change makes it so that we wait for dlv to declare that it's ready to accept connections before attempting to connect.

I've removed the calls to connectClient from stderr completely, as there is to my knowledge only two possible situations, both which are covered:

  • Dlv starts up fine, and emits the "Api server listening" message
  • Dlv crashes/exits, which gets caught by the error and close listeners.

@DMarby
Copy link
Contributor Author

DMarby commented Nov 19, 2017

Unsure as to why an unrelated test is failing on one specific go version on Mac, maybe someone with access could restart that travis job?

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Nov 20, 2017

The travis job has been restarted and it passed

This caused VS Code to attempt to connect to dlv the api server was listening.

Do you mean VS Code was trying to connect to delve before the api server was listening?

});
this.debugProcess.stdout.on('data', chunk => {
let str = chunk.toString();
if (this.onstdout) { this.onstdout(str); }
if (!serverRunning) {
if (!serverRunning && str.startsWith('API server listening at: ')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@derekparker Is there any other recommended way for editors to figure out when to connect the client to dlv other than waiting for the message API server listening at: on the stdout?

Copy link
Contributor

Choose a reason for hiding this comment

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

The editor should know the host / port it is trying to connect to, would it be possible to simply retry (potentially w/ backoff, or an upper-bound retry time) the connection until the server is running and accepting connections?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's possible and has come up as one of the solutions when I was discussing with @roblourens
Thanks @derekparker

@DMarby
Copy link
Contributor Author

DMarby commented Nov 20, 2017

Do you mean VS Code was trying to connect to delve before the api server was listening?

Yes, that is indeed what was happening.

@ramya-rao-a
Copy link
Contributor

@DMarby I am a little uncomfortable with checking for the string "API server listening at:".

The assumption here is that when delve returns with API server listening at:, it is ready for the client to make the connection.

Does delve ever return with any other kind of data before returning API server listening at:?
If not, then the existing code should also work, correct?
If yes, there are 2 things that could go wrong

  • The message itself may change in the future versions of delve
  • The data received in this.debugProcess.stdout.on('data', chunk => {}) is not guaranteed to be the complete string you expect. It could be just API server and listening at may come in the next chunk.

Also, @lukehoban do you remember why the delay was added before making the connection to delve?
See 69f7815
Would waiting for "API server listening at:" solve the issue you were trying to solve?

@DMarby
Copy link
Contributor Author

DMarby commented Nov 21, 2017

That makes sense! As far as I can tell, nothing is outputted on stdout before the API server listening, so I've removed the string comparison.

A better long-term solution would most likely be to attempt connection multiple times like @derekparker suggested, but I think this is a good change until that can be done.

@ramya-rao-a
Copy link
Contributor

@DMarby Below is the net change in this PR

  • Remove attempting to make a connection when dlv sends something on the stderr
  • Remove the 200ms wait right after the first data chunk is received in stdout before making the connection

I agree with the first point.
But how is removing the 200ms wait going to help #473?

@DMarby
Copy link
Contributor Author

DMarby commented Nov 21, 2017

The issue in #473 occurs when we attempt to connect to dlv before it's ready.
This occurs when we attempt to connect before the api server is listening, when we get stderr output. (Or when there's custom build flags with verbose causing stdout output during build, but that can only be resolved by adding behaviour such as retrying to connect several times I think)
As this change removes that behaviour, the 200ms delay shouldn't be needed anymore, as far as I can tell.

@ramya-rao-a
Copy link
Contributor

when there's custom build flags with verbose causing stdout output during build,

In that case, our assumption that " nothing is outputted on stdout before the API server listening," is wrong and we will have to add the check on "API server listening" back, correct?

@DMarby
Copy link
Contributor Author

DMarby commented Nov 22, 2017

After looking into it further (and testing by adding "-v" as buildFlags to my debug configuration), that gets outputted on stderr rather than stdout as well, so we shouldn't need the api server listening check.

@ramya-rao-a
Copy link
Contributor

In that case, I would keep the delay as well until we hear back from @lukehoban about the reason the delay was put in the first place.
The removal of the connection attempt during stderr should fix #473

@DMarby
Copy link
Contributor Author

DMarby commented Nov 23, 2017

Fair enough, I've gone ahead and reverted the change that removed the delay.

@ramya-rao-a ramya-rao-a merged commit ef53221 into microsoft:master Nov 23, 2017
@ramya-rao-a
Copy link
Contributor

Awesome, thanks for working on this @DMarby !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants