Adding DBGp Proxy support #280

SidIcarus · 2018-06-25T20:40:42Z

Related #107, #110 , #111, & #274
successfully allows for multiple concurrent connections to a double nat'd network

Handle response data
tests
- errors: timeout, failure to resolve, duplicate key, no key
- successful registration, de-registration
~~proxy error command~~

codecov · 2018-06-25T20:57:57Z

Codecov Report

Merging #280 into master will increase coverage by 0.37%.
The diff coverage is 70.37%.

@@            Coverage Diff             @@
##           master     #280      +/-   ##
==========================================
+ Coverage   70.11%   70.49%   +0.37%     
==========================================
  Files           5        6       +1     
  Lines        1014     1088      +74     
  Branches      161      182      +21     
==========================================
+ Hits          711      767      +56     
- Misses        303      321      +18

Impacted Files	Coverage Δ
src/terminal.ts	`9.37% <0%> (ø)`	⬆️
src/xdebugConnection.ts	`84.53% <100%> (ø)`	⬆️
src/phpDebug.ts	`65.61% <23.52%> (-1.33%)`	⬇️
src/proxyConnect.ts	`89.65% <89.65%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7280b6...99ee211. Read the comment docs.

felixfbecker

This looks much cleaner!

src/xdebugConnection.ts

src/phpDebug.ts

SidIcarus · 2018-06-29T20:23:41Z

@felixfbecker i think its ready for integration

We also needed to add some code to the python proxy. We are forwarding it to the github owner of komodo so he can forward it to their repo maintainers as they don't have a public facing repo for it.

SidIcarus · 2018-07-02T17:42:58Z

@felixfbecker
We haven't implemented support for the proxyerror command.

It currently will log this in xdebug.remote_log

<- proxyerror -i 1 -- sessionHash ->
<response xmlns="urn:debugger_protocol_v1"
    xmlns:xdebug="http://xdebug.org/dbgp/xdebug" command="proxyerror" transaction_id="1">
    <error code="4">
        <message>
            <![CDATA[unimplemented command]]>
        </message>
    </error>
</response>

Would you have any time to look into implementing that?
If not, I can do so just not immediately; is it fine to get this PR integrated without it?

felixfbecker · 2018-07-02T18:03:00Z

Realistically I am too short on time and personal benefits from this project to invest actual coding into it anymore, but I'll take some time to review. What is the proxyerror command for?

SidIcarus · 2018-07-02T18:21:37Z

Understandable.
They are error messages that the proxy would emit itself.

msg = "No server with key [%s], stopping request [%r]"% (idekey, session)
...
msg = "Unable to connect to server with key [%s], stopping request [%r]"% (idekey, session)
...
msg = "outputHandler Closing connection, invalid protocol used"
...
msg = "responseHandler Closing connection, invalid protocol used"
...
msg = "Unable to connect to the server listener %s:%d [%r]" % \
...
msg = "Closing connection, invalid protocol used"
...
error(msg)
...
    def error(self, msg):
        if not self._socket:
            log.error(msg)
            return
        self.sendCommand(['proxyerror'], msg)
...
    def sendCommand(self, argv, data = None):
        if not self._socket:
            raise DBGPError('Socket disconnected', ERROR_EXCEPTION)
        self._transaction_id = self._transaction_id + 1

        argv += ['-i', str(self._transaction_id)]
        if data:
            argv += ['--', base64.encodestring(data).strip()]
            
        # if the args need quoting, it will happen here, argv2line doesn't
        # handle it all
        # cmdline = listcmd.argv2line(argv)
        escapedArgs = []
        for arg in argv:
            # we must escape any quotes in the argument
            arg = self._re_escape.sub(r'\\\1', arg)
            if ' ' in arg or '"' in arg or "'" in arg or '\\' in arg:
                arg = '"'+arg+'"'
            escapedArgs.append(str(arg))
        cmdline = ' '.join(escapedArgs)
        try:
            #print "sendCommand: %s"% cmdline
            log.debug("sendCommand: %s", cmdline)
            self._socket.sendall(cmdline+'\0')
            #log.debug("sendCommand: %s DONE", cmdline)
        except socket.error as e:
            log.error("session sendCommand socket error %r", e)
            self._stop = 1
        return self._transaction_id

felixfbecker · 2018-07-02T19:39:33Z

I think error handling is important or its gonna create a maintenance burden when users don't know why something is not working and will file issues.
It shouldn't be hard to implement, right?

towhidabsar · 2018-07-03T17:30:04Z

Hi @felixfbecker , I looked into implementing the fix for proxyerror but it seems our first assumption was wrong. proxyerroris sent to XDebug server and not the IDE. As a result, we don't need to handle it in the debugger. So the code should be complete as is.
Please review it and let us know if there is anything else of concern.

towhidabsar · 2018-07-06T13:37:37Z

@felixfbecker Did you get a chance to review the code?

SidIcarus · 2018-07-23T14:38:29Z

@felixfbecker when do you think you'll be able to integrate this PR in?

felixfbecker

Sorry I was on vacation. See comments

src/proxyConnect.ts

felixfbecker · 2018-07-26T00:00:01Z

src/proxyConnect.ts

+        this._key = key
+        this._port = port
+        this._timeout = timeout
+        this.msgs = new Map([


Using a Map with string identifiers for this means it is not typesafe. When these are only used once, I would inline these. If they are needed multiple times, define a method that returns it.

How is it not typesafe? It specifies type in its declaration on line 25: public msgs: Map<string, string>.
A method would justifiable if the msgs had a way to change but there isn't and they don't.
Using a map allowed us to group their intended functionality whilst reducing variable name verbosity.

When calling msgs.get() you could easily have a typo in the key. You also get no autocompletion for the keys.

An object would also be fine.

public msgs:{ [key:string]: string } = {} constructor() { ... this.msgs = { defaultError: 'Unknown Error', deregisterInfo: `Deregistering ${this._key} with proxy @ ${this._host}:${this._port}`, deregisterSuccess: 'Deregistration successful', duplicateKey: 'IDE Key already exists', nonexistentKey: 'No IDE key', registerInfo: `Registering ${this._key} with proxy @ ${this._host}:${this._port}`, registerSuccess: 'Registration successful', resolve: `Failure to resolve ${this._host}`, timeout: `Timeout connecting to ${this._host}:${this._port}`, } ... }

This should cover it, yes?

No, because the key is still string. But if you move the object from the constructor to the initialiser instead and let the type be inferred, it would be safe.

@felixfbecker included an interface defining ProxyMessages to ensure the key is type safe.

src/proxyConnect.ts

felixfbecker · 2018-07-26T00:08:34Z

src/proxyConnect.ts

+    /** proxy response data parser */
+    private _parser = new DOMParser()
+    /** tcp connection to communicate with proxy server */
+    public _socket = new Socket()


Why is this public?

IIRC, when we began writing our tests we mocked out the socket and such but weren't completely successful. We opted to make the socket public and that allowed us to test each step.

The way the test uses this should-be private property to assert things looks very hacky to me, as it is making use of internals and testing implementation details. I am not confident I would be able to maintain and support this code if issues pop up. Please see if you can find a way to define clear interface boundaries and to test at those boundaries.

@felixfbecker I changed it back to private and followed the convention you used to pass in the socket in the constructor. Please let me know if this is acceptable.

src/phpDebug.ts

SidIcarus · 2018-07-30T22:13:44Z

I've rebased our fork and resolved the files in conflict. Once the current comments are resolved, I can push that up.

felixfbecker · 2018-08-13T20:47:57Z

I replied a week ago, please take a look at my questions above

towhidabsar · 2018-09-13T14:27:22Z

@felixfbecker please take a look. Updated the branch to have all the builds pass, and re-tested the code.

…ivate - new ProxyMessage interface to ensure typesafe messages - socket is now private and can be passed through constructor

towhidabsar · 2019-05-02T16:26:46Z

@felixfbecker ping. Please let me know if the recent commits address your concerns?

bilogic · 2019-05-11T13:44:06Z

it's odd how slowly this moves.

nn-hh · 2019-06-07T14:09:24Z

@felixfbecker Please.

bilogic · 2019-06-19T02:01:52Z

@SidIcarus can you provide some instructions on how to load your extension in vscode?

bilogic · 2019-07-02T12:35:44Z

has everyone given up?

X-Coder · 2019-07-02T20:24:06Z

Hope not, because its good work and very useful for me.

@bilogic For my smaller customizations I installed the extensions as usual and manually patched the files in C:\Users%USERNAME%.vscode\extensions\felixfbecker.php-debug-1.13.0\out.
This is not the best way and needs to be done each time the extensions updates.

The files from SidIcarus are source files written in TypeScript and these needs to be compiled into normal javascript before you can use them.

towhidabsar · 2019-07-02T20:41:56Z

Hi, sorry @SidIcarus and I are still waiting on @felixfbecker to check out and give us feedback on the latest commits.
In the meantime if you want to try out the extension, you can just clone my repository and do the following:
npm run build
npm install -g vsce if you don't have the vsce package
vsce package inside the folder will generate the vsix file
Then just install the extension using the generated VSIX in vscode.
But hopefully we can get this merged in as soon as possible.

zobo · 2019-07-15T13:06:05Z

@felixfbecker I have created a PR to the forked repo with some fixes and extensions. Since there was no response in over a week, I'm linking it here and if needed I can rebase things an create a new PR here. Link: towhidabsar#1

BurningDog · 2019-11-15T10:52:30Z

This would be super useful to myself and my team, and for anyone running php inside kubernetes/docker.

What would the next step be here? Are we waiting for responses from @felixfbecker?

zobo · 2019-11-15T13:05:15Z

Seems so... I tried reaching out, but got no response. I'll probably fork and re-publish the plugin on the store just so that our team can use these features.

felixfbecker

Looks well-implemented. I won't be able to maintain this though because I don't use a proxy. Would anyone be willing to take maintenance of this part of the codebase after merge?

felixfbecker · 2019-11-15T13:12:22Z

@zobo what about your PR towhidabsar#1 it block merging this?

zobo · 2019-11-15T13:43:08Z

Hi @felixfbecker I believe it got stuck there also. I can reach out again, but could also just make a PR against the merged code. I think there are some issues with the PR without my latest changes. Let me check and get back to you.

felixfbecker · 2019-11-15T13:44:00Z

You could just open the PR here including both this PR and your fixes

zobo · 2019-11-15T13:52:25Z

Ok, I'll make a new PR.

zobo · 2019-11-15T14:03:38Z

Created #378 that should be merged after this one. I plan to open at least one more proxy related PR, but not right away.
There are however some other PRs that could use some love :)

felixfbecker · 2019-11-16T12:35:26Z

@SidIcarus it seems that the changes you made to install PHP on macOS in Travis ar e no longer working and the build is failing now :/
Do you have an idea why?

MurzNN · 2020-09-28T11:35:11Z

Seems this PR is still not merged, what is the current stoppers for finish and merge this PR?

zobo · 2021-11-03T22:56:52Z

PR #685 re-implements this. The base was sadly so far off that I could not merge and fix it.

felixfbecker reviewed Jun 25, 2018

View reviewed changes

src/xdebugConnection.ts Outdated Show resolved Hide resolved

src/xdebugConnection.ts Outdated Show resolved Hide resolved

src/phpDebug.ts Outdated Show resolved Hide resolved

felixfbecker changed the title ~~Adding DBGp Proxy support~~ WIP: Adding DBGp Proxy support Jun 25, 2018

SidIcarus force-pushed the master branch 2 times, most recently from 6e2d704 to 0b25edb Compare June 29, 2018 20:11

SidIcarus changed the title ~~WIP: Adding DBGp Proxy support~~ Adding DBGp Proxy support Jun 29, 2018

SidIcarus force-pushed the master branch 2 times, most recently from ac9c919 to 7b33cf2 Compare July 2, 2018 17:34

SidIcarus force-pushed the master branch from 7b33cf2 to 9b06bd2 Compare July 2, 2018 17:56

SidIcarus force-pushed the master branch from 9b06bd2 to aa5424f Compare July 2, 2018 18:13

felixfbecker reviewed Jul 26, 2018

View reviewed changes

SidIcarus commented Jul 30, 2018

View reviewed changes

src/phpDebug.ts Show resolved Hide resolved

SidIcarus force-pushed the master branch 2 times, most recently from 4421096 to 60d28bc Compare August 15, 2018 15:42

SidIcarus force-pushed the master branch 5 times, most recently from 38a4712 to b96010d Compare October 15, 2018 16:14

refactor(option): use bool v. num to improve clarity in the options

0c1ba35

SidIcarus force-pushed the master branch from c3381d4 to 0c1ba35 Compare April 29, 2019 17:13

towhidabsar added 3 commits April 29, 2019 14:30

fix(proxyConnect): add ProxyMessages interface and make socket pr…

8be4919

…ivate - new ProxyMessage interface to ensure typesafe messages - socket is now private and can be passed through constructor

fix(proxyConnect): run prettier

cafeeec

fix(proxy): remove console.log

99ee211

zobo mentioned this pull request Nov 15, 2019

not working with remote-ssh extension #377

Closed

felixfbecker approved these changes Nov 15, 2019

View reviewed changes

felixfbecker added the feature label Nov 16, 2019

zobo mentioned this pull request Nov 16, 2019

Add DBGP proxy support, with client local port #378

Closed

MurzNN mentioned this pull request Sep 28, 2020

Get his Pull requests robberphex/vscode-php-debug#2

Open

felixfbecker changed the base branch from master to main December 21, 2020 20:20

zobo closed this Nov 3, 2021

Adding DBGp Proxy support #280

Adding DBGp Proxy support #280

Conversation

SidIcarus commented Jun 25, 2018 • edited

codecov bot commented Jun 25, 2018 • edited by ghost

Codecov Report

felixfbecker left a comment

Choose a reason for hiding this comment

SidIcarus commented Jun 29, 2018

SidIcarus commented Jul 2, 2018

felixfbecker commented Jul 2, 2018

SidIcarus commented Jul 2, 2018 • edited

felixfbecker commented Jul 2, 2018

towhidabsar commented Jul 3, 2018 • edited

towhidabsar commented Jul 6, 2018

SidIcarus commented Jul 23, 2018

felixfbecker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SidIcarus Jul 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SidIcarus Aug 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SidIcarus commented Jul 30, 2018

felixfbecker commented Aug 13, 2018

towhidabsar commented Sep 13, 2018

towhidabsar commented May 2, 2019

bilogic commented May 11, 2019

nn-hh commented Jun 7, 2019

bilogic commented Jun 19, 2019

bilogic commented Jul 2, 2019

X-Coder commented Jul 2, 2019

towhidabsar commented Jul 2, 2019

zobo commented Jul 15, 2019

BurningDog commented Nov 15, 2019

zobo commented Nov 15, 2019

felixfbecker left a comment

Choose a reason for hiding this comment

felixfbecker commented Nov 15, 2019

zobo commented Nov 15, 2019

felixfbecker commented Nov 15, 2019

zobo commented Nov 15, 2019

zobo commented Nov 15, 2019

felixfbecker commented Nov 16, 2019

MurzNN commented Sep 28, 2020

zobo commented Nov 3, 2021

SidIcarus commented Jun 25, 2018 •

edited

codecov bot commented Jun 25, 2018 •

edited by ghost

SidIcarus commented Jul 2, 2018 •

edited

towhidabsar commented Jul 3, 2018 •

edited

SidIcarus Jul 30, 2018 •

edited

SidIcarus Aug 14, 2018 •

edited