Add ability to attach, detach and attach once again by DonJayamanne · Pull Request #287 · microsoft/ptvsd

DonJayamanne · 2018-03-30T23:26:33Z

@int19h @ericsnowcurrently @karthiknadig
The code is far from complete., but would like some early feedback on the solution.
With the changes I've made I've managed to get re-attach working, at least on VSC.

Pending - Fix remainder of the code and what ever tests fail

Summary:

Create new threading to check for connections
Change how callbacks are passed into pydevd

DonJayamanne · 2018-03-30T23:27:35Z

ptvsd/wrapper.py

-#    print(s)
-#ipcjson._TRACE = ipcjson_trace
+def ipcjson_trace(s):
+    with open('/Users/donjayamanne/Desktop/Development/vscode/my_forked_ptvsd/ptvsd/log.log', 'a') as file:


Temporary debugging code 😃

DonJayamanne · 2018-03-30T23:29:09Z

ptvsd/wrapper.py


    if addhandlers:
        _add_atexit_handler(proc, server_thread)
        _set_signal_handlers(proc)


I'm going to have to modify _set_signal_handlers to close all VS Code message processors.

DonJayamanne · 2018-03-30T23:32:49Z

ptvsd/wrapper.py

+            client, _ = server.accept()
+            _add_socket_handler(client, pydevd, 'ptvsd.Server', killonclose=True, addhandlers=False)
+
+    connection_thread = threading.Thread(target=_wait_for_more, name='ptvsd_client_connection')


New thread that'll ensure we support more than one client.
@int19h When speaking to Karthik the question about limiting multiple concurrent client connections came up.
Should we have a limit on the number of concurrent connections, to just one?

Yes. We never supported anything more than that before, and just thinking of all the possibilities of things going wrong in that scenario makes my head hurt. :)

DonJayamanne

Some comments

karthiknadig · 2018-04-02T16:51:37Z

ptvsd/wrapper.py

+            _add_pydevd_event_handler(client, pydevd, name='ptvsd.Server')
+
+    connection_thread = threading.Thread(target=_wait_for_more_connections,
+                                         name='ptvsd_client_connection')


Use ptvsd.Client.Connection as name, to be consistent with other ptvsd thread names

karthiknadig · 2018-04-02T17:58:01Z

ptvsd/wrapper.py

    """
    client = _create_client()
    client.connect((host, port))
+    pydevd = PydevdSocket()


That is not needed, right?

DonJayamanne · 2018-04-02T22:40:41Z

@ericsnowcurrently
Here are the requirements for remote debugging:

Current functionality of PR
Run a program
- Somewhere down the line, we can import ptvsd and start the socket server (PTVSD)
- It should listen for connections (non-blocking)
  This is a scenario where we have the socket server listening for connections, but in a non-blocking manner.
- If a connection is made, then debugger should connect
If more than one try to connect, we need to return an error message rather than just rejecting the connection (else user will never know why the connection is being rejected - very important for us too to diagnoze the errors)

codecov-io · 2018-04-02T22:59:11Z

Codecov Report

Merging #287 into master will increase coverage by 0.23%.
The diff coverage is 73.01%.

@@            Coverage Diff             @@
##           master     #287      +/-   ##
==========================================
+ Coverage    57.4%   57.64%   +0.23%     
==========================================
  Files          10       10              
  Lines        1850     1891      +41     
==========================================
+ Hits         1062     1090      +28     
- Misses        788      801      +13

Impacted Files	Coverage Δ
ptvsd/ipcjson.py	`65.7% <100%> (+0.33%)`	⬆️
ptvsd/wrapper.py	`50.9% <72.13%> (+0.6%)`	⬆️

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 78d3225...d059ca8. Read the comment docs.

DonJayamanne · 2018-04-03T04:30:07Z

Closing in favor of #296

DonJayamanne requested review from ericsnowcurrently and int19h March 30, 2018 23:26

DonJayamanne commented Mar 30, 2018

View reviewed changes

karthiknadig reviewed Apr 2, 2018

View reviewed changes

DonJayamanne changed the title ~~Add ability to attach, detach and attach once again~~ WIP - Add ability to attach, detach and attach once again Apr 2, 2018

DonJayamanne force-pushed the detach branch from 5807ece to b865d22 Compare April 2, 2018 17:50

karthiknadig reviewed Apr 2, 2018

View reviewed changes

DonJayamanne added 12 commits April 2, 2018 12:43

initial commit

03083c1

refactor with signal handlers

7d11fe6

more cleanup required

ecbb0b4

fix tests

2b40d99

undo unecessary formatting

574b7ed

undo more unnecessary formatting

7a607e0

remove unwanted arg

aa50261

code review fixes

fbce1db

refactor code

8da8783

fixed test

2d6ec0a

send terminated event

23c0bb3

fixed attach tests

a762f27

DonJayamanne force-pushed the detach branch from 3fb4b40 to a762f27 Compare April 2, 2018 19:44

DonJayamanne added 3 commits April 2, 2018 12:48

capture info before replying back in attach request

b5e31aa

change message

418d489

remove duplicate initialization

2c3976d

DonJayamanne changed the title ~~WIP - Add ability to attach, detach and attach once again~~ Add ability to attach, detach and attach once again Apr 2, 2018

DonJayamanne mentioned this pull request Apr 2, 2018

Re-attach does not work using experimental debugger microsoft/vscode-python#1255

Closed

fixed format error

d059ca8

DonJayamanne mentioned this pull request Apr 3, 2018

Breakpoint not hit after re-attaching to remote debugger #295

Closed

ericsnowcurrently mentioned this pull request Apr 3, 2018

Refactor before fixing support for re-attach. #296

Merged

DonJayamanne closed this Apr 3, 2018

DonJayamanne deleted the detach branch July 14, 2018 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to attach, detach and attach once again#287

Add ability to attach, detach and attach once again#287
DonJayamanne wants to merge 16 commits intomicrosoft:masterfrom
DonJayamanne:detach

DonJayamanne commented Mar 30, 2018 •

edited

Loading

Uh oh!

DonJayamanne Mar 30, 2018

Uh oh!

DonJayamanne Mar 30, 2018

Uh oh!

DonJayamanne Mar 30, 2018

Uh oh!

int19h Mar 30, 2018

Uh oh!

DonJayamanne left a comment

Uh oh!

karthiknadig Apr 2, 2018

Uh oh!

karthiknadig Apr 2, 2018

Uh oh!

DonJayamanne Apr 2, 2018

Uh oh!

DonJayamanne commented Apr 2, 2018 •

edited

Loading

Uh oh!

codecov-io commented Apr 2, 2018

Uh oh!

DonJayamanne commented Apr 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

DonJayamanne commented Mar 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DonJayamanne Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

int19h Mar 30, 2018

Choose a reason for hiding this comment

Uh oh!

DonJayamanne left a comment

Choose a reason for hiding this comment

Uh oh!

karthiknadig Apr 2, 2018

Choose a reason for hiding this comment

Uh oh!

karthiknadig Apr 2, 2018

Choose a reason for hiding this comment

Uh oh!

DonJayamanne Apr 2, 2018

Choose a reason for hiding this comment

Uh oh!

DonJayamanne commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 2, 2018

Codecov Report

Uh oh!

DonJayamanne commented Apr 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DonJayamanne commented Mar 30, 2018 •

edited

Loading

DonJayamanne commented Apr 2, 2018 •

edited

Loading