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

Debugger: current multi clients system is unuseful #858

Closed
3y3 opened this issue Feb 16, 2015 · 8 comments
Closed

Debugger: current multi clients system is unuseful #858

3y3 opened this issue Feb 16, 2015 · 8 comments

Comments

@3y3
Copy link

3y3 commented Feb 16, 2015

Current protocol bases on seq, request_seq counters.

Pseudo code. Send backtrace command:

debug_connection.request({
  type: 'command',
  command: 'backtrace',
  seq: 5,
  attributes: {...}
}, function(res) {
  // res.request_seq == 5
  processResponse(res);
});

Ok. It works fine.

What about Alice and Bob?
Alice tries to debug app and she pauses in random place of code. Her seq couter is 4 (Alice.seq=4).
She sends a backtrace command and waits for response with request_seq=4.

In this moment Bob also sends request to debugger. He sends lookup request with Bob.seq=4.
And "WOW! So fast lookup!" - thinks Bob, starts to parse response... "But wait! WHAT!"

res = {
  request_seq: 4,
  command: 'backtrace'
}

"It's not for my request!"

From _debug_agent:

function Agent() {
  net.Server.call(this, this.onConnection);

  this.first = true;
  this.binding = process._debugAPI;

  var self = this;
  this.binding.onmessage = function(msg) {
    self.clients.forEach(function(client) {
      client.send({}, msg);
    });
  };

  this.clients = [];
  assert(this.binding, 'Debugger agent running without bindings!');
}

Are you see here a problem? =)

What I think about this:
Multi clients is a cool feature for debugger, but it over complicates the protocol realisation, and I do not know a situation where we need two or more debugger clients.

Is there a main discussion about new debugger?
I'm ready to work on some issues after discussion. Some other is out of my competence at this time.
Some other issues:
#781
nodejs/node-v0.x-archive#9156

@ofrobots
Copy link
Contributor

For the 'main discussion about new debugger':

  • V8's JSON-based debug protocol is supposed to be deprecated. It would be better to use debug-debugger.js directly, or, better, use the C++ interface.

@3y3
Copy link
Author

3y3 commented Mar 19, 2015

@ofrobots , can you give me link on main discussion about deprecation in V8?

@ofrobots
Copy link
Contributor

nodejs/node-v0.x-archive#7473 is relevant. However, I don't think there any discussion that spells this out. I don't speak on behalf of the V8 team, but that's the feeling I have given that Chrome doesn't use the JSON api. I think one of the first steps in the deprecation was to remove the debug agent from V8.

I think @bmeck has some context about this.

@bmeck
Copy link
Member

bmeck commented Mar 20, 2015

Chrome does not use the V8 debugger protocol anymore, they use a bridge to get the debugger context directly ( https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/debug-debugger.js&q=debug-debugger&sq=package:chromium&type=cs ) via v8::Debugger::GetContext , from they they actually create a new debugger protocol, the Chrome devtools protocol ( https://developer.chrome.com/devtools/docs/debugger-protocol ).

There is an effort to move the Chrome devtools protocol out of Chromium right now so that Node/IO.js can use that instead of trying to recreate devtools efforts. I am currently trying to see if they have a public ticket about this, all my communication has been via email.

As for the problem of multiple clients, https://developer.chrome.com/devtools/docs/debugger-protocol#simultaneous , those are still unaccepted and will most likely stay this way. This has some interesting reasoning, but mainly involves around concurrent side effects on the VM. I doubt it will change in the near future, and I am not sure it is safe to support multiple clients anyway.

@Trott
Copy link
Member

Trott commented Feb 4, 2016

@bmeck @ofrobots Is this something that should be closed in your opinion? If not, is it a sufficiently distant feature request that it makes sense to move it to nodejs/NG or something like that?

@bnoordhuis
Copy link
Member

I think this can be closed. Having multiple people debug the same application seems sufficiently niche to me it's not worth bothering. And that's without going into what it would even mean to have two or more people place breakpoints, evaluate code, etc. in a single-threaded application.

@3y3
Copy link
Author

3y3 commented Feb 4, 2016

@bnoordhuis , I'm not sure that you understand correctly this issue.
At current time we have multi client debugger in node, but we don't have any tools to resolve conflicts in request_seq param. All responses works in multicast mode. So I can send request to debugger with seq: 1 and receive two responses with request_seq: 1. If we want to save multiclient system, we need to delegate a namespace in debug-agent for each user connection, and send this namespace in connection response. Then we can switch on something like seq: 'BobNS:1' -> request_seq: 'BobNS:1'.

But I propose to completely deprecate multiclient system and move this feature to external tools, like node-inspector.

Here is my outdated commit where I fix this and other problems #1977

@bnoordhuis
Copy link
Member

You're proposing to remove the ability to connect two or more clients? That wasn't entirely clear to me from your original message.

I guess I still don't see the need to do anything. Yes, more than one client doesn't work, but so what? When do people actually do that?

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 a pull request may close this issue.

6 participants