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

Reverse the connection direction used by enable attach internals. #1833

Merged
merged 3 commits into from Oct 9, 2019

Conversation

@karthiknadig
Copy link
Member

commented Oct 9, 2019

Fixes #1830

@karthiknadig karthiknadig requested a review from int19h Oct 9, 2019
@karthiknadig karthiknadig force-pushed the karthiknadig:fixattach branch from 73f2fe1 to 77d2190 Oct 9, 2019
@int19h
int19h approved these changes Oct 9, 2019
@karthiknadig karthiknadig marked this pull request as ready for review Oct 9, 2019
@karthiknadig karthiknadig requested a review from fabioz as a code owner Oct 9, 2019
@karthiknadig karthiknadig merged commit c6cee27 into microsoft:master Oct 9, 2019
3 of 4 checks passed
3 of 4 checks passed
ptvsd-testing-automation #20191008.4 failed
Details
SonarCloud Code Analysis Quality Gate passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details
@@ -2253,6 +2253,7 @@ def _enable_attach(
patch_multiprocessing=False,
access_token=None,
ide_access_token=None,
as_client=False

This comment has been minimized.

Copy link
@fabioz

fabioz Oct 9, 2019

Collaborator

I know this is already merged, but I don't think we should do this change on pydevd... the API to connect as a client is calling settrace directly with the proper host/port, whereas _enable_attach() was really only meant to be called to act as a server and wait until a connection was done, so, I think the changes on pydevd should be reverted and ptvsd should just use settrace directly here (the code related to the checking the port is also not interesting in this case... if it's starting a server it should be checked, but if it'll connect to a port in the client it could connect one time to one port and later on connect to another port).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.