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

support arbitrary requests, not only "launch" and "attach" #1697

Closed
weinand opened this issue Dec 29, 2015 · 22 comments
Closed

support arbitrary requests, not only "launch" and "attach" #1697

weinand opened this issue Dec 29, 2015 · 22 comments
Assignees
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code
Milestone

Comments

@weinand
Copy link
Contributor

weinand commented Dec 29, 2015

In addition to "launch" and "attach" we should support any request, e.g. "listen"
(php-debug uses a "listen" model).

@weinand weinand added feature-request Request for new features or functionality debug Debug viewlet, configurations, breakpoints, adapter issues labels Dec 29, 2015
@weinand
Copy link
Contributor Author

weinand commented Dec 29, 2015

CC @felixfbecker

@felixfbecker
Copy link
Contributor

👍

@felixfbecker
Copy link
Contributor

Currently I'm abusing launch for this. But this has some issues. For example, when I launch, and then set a breakpoint before I have any connection with the debugger engine, VS Code sends me a setBreakpointsRequest that I cannot execute. I can only save the breakpoints and apply them later, but then I cannot validate the breakpoints and notify VS Code if some breakpoint was not valid. This problem should be resolved by a listen setting (VS Code should only send breakpoints after I sent some kind of ConnectionEvent).

Also, I'm currently abusing the Threads feature to allow multiple simultaneous connections to be debugged. Think about a PHP script that does multiple AJAX requests to the same PHP backend in parallel. This works only because PHP does not support real threads, but with java server pages this would not work. On every new connection I apply the breakpoints I got once for my main connection. In a listen model VS Code should instead send me the info for the new connection on a ConnectionEvent and should display multiple connections as an additional level over threads so I don't have to abuse threads for this.

@weinand
Copy link
Contributor Author

weinand commented Dec 29, 2015

@felixfbecker VSCode only sends you a setBreakpointsRequest as a response to the initialized event that the php-debug adapter is emitting. So you are in control! Try to send the initialized event if your debug adapter is ready to receive setBreakpointsRequests. No "listen" is required for this. We will introduce a "listen" only to give you a better name. But the flow of events will not change.

@weinand
Copy link
Contributor Author

weinand commented Dec 29, 2015

@felixfbecker VSCode does not know any difference between "launch" or "attach" (and in the future "listen"). It is the debug protocol that should deal with the differences (example: an initialized event triggers VSCode to send all the breakpoints). If we need something new for threads (or pseudo-threads) we can work on this together...

@weinand
Copy link
Contributor Author

weinand commented Dec 29, 2015

@felixfbecker Below you can find the corresponding section of the Debugger API doc from the VSCode web site. IMO it clearly explains how the "initialize event" and the "setBreakpoints" request interacts. Since you haven't been able to use this information, the documentation seems to be lacking. Do you have any suggestions how to improve this?

"Since VS Code persists breakpoints on behalf of the debug adapter, it has to register the breakpoints with the debug adapter when a session starts. Since VS Code does not know when is a good time for this, the debug adapter is expected to send an initialize event to VS Code to announce that it is ready to accept breakpoints. VS Code will then send all breakpoints by calling setBreakpoints and setExceptionBreakpoints in return. So don't forget to send the initialize event when you are ready to accept breakpoints. Otherwise persisted breakpoints are not restored."

@felixfbecker
Copy link
Contributor

@weinand This currently is implemented in php-debug:

  • user sets breakpoint
  • user hits F5
  • debug engine connects
  • debug adapter sends InitializedEvent
  • VS Code sends breakpoints

But think about this scenario

  • user hits F5
  • user sets breakpoints before debug engine connects
  • VS Code sends setBreakpointRequest before InitializedEvent
  • debug adapter has no way to validate the breakpoints - I therefor return verified: false for all, all breakpoints turn grey
  • debug engine connects
  • debug adapter sends InitializedEvent
  • VS Code sends breakpoints again, this time I can verify them

It is not that big of an issue though.

@weinand
Copy link
Contributor Author

weinand commented Dec 29, 2015

@felixfbecker good scenario! Just let VSCode buffer all new breakpoints until the InitializedEvent is received.

@isidorn we should fix this. I've created #1702 for this.

@felixfbecker
Copy link
Contributor

@weinand How will debugadapter-node handle the listen or arbitrary launch types?

@bpasero bpasero added this to the Backlog milestone Dec 30, 2015
@weinand
Copy link
Contributor Author

weinand commented Dec 30, 2015

@felixfbecker probably by using some reflection: if your subclass of DebugAdapter implements a method "listenRequest" and if your configuration schema has an entry for a "listen" request, "listenRequest" will be called.

@felixfbecker
Copy link
Contributor

like this?

this[args.type + 'Request'](response, args);

@weinand
Copy link
Contributor Author

weinand commented Dec 30, 2015

Basically yes.

@isidorn
Copy link
Contributor

isidorn commented Jan 4, 2016

Hi,

We have fixed the breakpoint issue described in #1702

As for the launch and attach requests VSCode has special knowledge about these two. For example, the "restart" and "stop" actions get renamed to "reconnect" and "disconnect" in the attach case.

If I understand correctly in the listen scenario you would need a special way to show connections in the UI, thus listen would also be special and not just an arbitary request.
For the moment you should abuse the threads for showing connections and if we want to support listen we should have a plan item for it imho

@isidorn isidorn removed their assignment Jan 4, 2016
@felixfbecker
Copy link
Contributor

@isidorn Yes, in the optimal case listen would be special and there would be some way to show active connections in the UI without abusing threads. But for my use cases so far, the current way with threads works fine because PHP doesnt support threads anyway - it only doesnt work that well because of #1701 and #1703. But I think it would also be nice if "listen" was simply supported as a value for type without any special handling, because it just fits better than "launch".

Btw, @weinand, can an adapter send an InitializedEvent multiple times to request breakpoints again at a later time?

@weinand
Copy link
Contributor Author

weinand commented Jan 4, 2016

@felixfbecker yes, sending InitializedEvent multiple times is supported.

@isidorn
Copy link
Contributor

isidorn commented Jan 4, 2016

#1701 and #1703 should get fixed soon, we are looking into improving our threading model.
Accidently you can put "listen" in your launch.json and everything will work fine except that you will get warnings in your launch.json

@felixfbecker
Copy link
Contributor

@weinand that is awesome, will make my code a lot simpler.

@isidorn Then how about just adding "listen" as valid value to launch.json schema and adding listenRequest to debugadapter-node?

@felixfbecker
Copy link
Contributor

Or allow overriding it in package.json, like

"properties": {
  "request": {
    "type": "string",
    "default": "listen"
  },

@vadimcn
Copy link
Contributor

vadimcn commented Oct 16, 2017

@isidorn: So what's the current thinking about extension-defined request types?

My adapter supports a custom request type, whose config properties are significantly different from either launch or attach, so it really deserves its own case in configuration schema. Unfortunately, it looks like VSCode has started enforcing that request can only be "launch" or "attach" :(

@isidorn
Copy link
Contributor

isidorn commented Oct 17, 2017

@vadimcn there are no immediate plans to support extension defined request types. Though I would let @weinand comment more on this one once he is back from vacation.

@weinand weinand self-assigned this Oct 25, 2017
@weinand
Copy link
Contributor Author

weinand commented Nov 13, 2017

Since this change can only happen in the 2.0 version of the debug protocol (which is not yet on the horizon).

@weinand weinand added the *out-of-scope Posted issue is not in scope of VS Code label Dec 8, 2017
@vscodebot
Copy link

vscodebot bot commented Dec 8, 2017

This issue is being closed to keep the number of issues in our inbox on a manageable level, we are closing issues that have been on the backlog for a long time but have not gained traction: We look at the number of votes the issue has received and the number of duplicate issues filed. If you disagree and feel that this issue is crucial: We are happy to listen and to reconsider.

If you wonder what we are up to, please see our roadmap and issue reporting guidelines.

Thanks for your understanding and happy coding!

@vscodebot vscodebot bot closed this as completed Dec 8, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debug Debug viewlet, configurations, breakpoints, adapter issues feature-request Request for new features or functionality *out-of-scope Posted issue is not in scope of VS Code
Projects
None yet
Development

No branches or pull requests

5 participants