Skip to content

Detect whether debugger is already running and skip connecting/listen… #1657

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.

Sign up for GitHub

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

Merged
rchiodo merged 1 commit into from
Aug 27, 2024
Merged

Detect whether debugger is already running and skip connecting/listen… #1657

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 32 additions & 17 deletions src/debugpy/server/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ def do(arg, it):
port = int(port)
except Exception:
port = -1
if not (0 <= port < 2 ** 16):
if not (0 <= port < 2**16):
raise ValueError("invalid port number")

options.mode = mode
Expand Down Expand Up @@ -191,23 +191,26 @@ def do(arg, it):
]
# fmt: on


# Consume all the args from argv
def consume_argv():
while len(sys.argv) >= 2:
value = sys.argv[1]
del sys.argv[1]
yield value


# Consume all the args from a given list
def consume_args(args: list):
if (args is sys.argv):
if args is sys.argv:
yield from consume_argv()
else:
while args:
value = args[0]
del args[0]
yield value


# Parse the args from the command line, then from the environment.
# Args from the environment are only used if they are not already set from the command line.
def parse_args():
Expand All @@ -232,20 +235,28 @@ def parse_args():
assert options.target_kind is not None
assert options.address is not None


def parse_args_from_command_line(seen: set):
parse_args_helper(sys.argv, seen)


def parse_args_from_environment(seenFromCommandLine: set):
args = os.environ.get("DEBUGPY_EXTRA_ARGV")
if (not args):
if not args:
return

argsList = args.split()

seenFromEnvironment = set()
parse_args_helper(argsList, seenFromCommandLine, seenFromEnvironment, True)

def parse_args_helper(args: list, seenFromCommandLine: set, seenFromEnvironment: set = set(), isFromEnvironment=False):

def parse_args_helper(
args: list,
seenFromCommandLine: set,
seenFromEnvironment: set = set(),
isFromEnvironment=False,
):
iterator = consume_args(args)

while True:
Expand All @@ -264,17 +275,17 @@ def parse_args_helper(args: list, seenFromCommandLine: set, seenFromEnvironment:
raise ValueError("unrecognized switch " + switch)

# if we're parsing from the command line, and we've already seen the switch on the command line, this is an error
if (not isFromEnvironment and switch in seenFromCommandLine):
if not isFromEnvironment and switch in seenFromCommandLine:
raise ValueError("duplicate switch on command line: " + switch)
# if we're parsing from the environment, and we've already seen the switch in the environment, this is an error
elif (isFromEnvironment and switch in seenFromEnvironment):
elif isFromEnvironment and switch in seenFromEnvironment:
raise ValueError("duplicate switch from environment: " + switch)
# if we're parsing from the environment, and we've already seen the switch on the command line, skip it, since command line takes precedence
elif (isFromEnvironment and switch in seenFromCommandLine):
elif isFromEnvironment and switch in seenFromCommandLine:
continue
# otherwise, the switch is new, so add it to the appropriate set
else:
if (isFromEnvironment):
if isFromEnvironment:
seenFromEnvironment.add(switch)
else:
seenFromCommandLine.add(switch)
Expand All @@ -291,9 +302,10 @@ def parse_args_helper(args: list, seenFromCommandLine: set, seenFromEnvironment:
# If we're parsing the command line, we're done after we've processed the target
# Otherwise, we need to keep parsing until all args are consumed, since the target may be set from the command line
# already, but there might be additional args in the environment that we want to process.
if (not isFromEnvironment and options.target is not None):
if not isFromEnvironment and options.target is not None:
break


def start_debugging(argv_0):
# We need to set up sys.argv[0] before invoking either listen() or connect(),
# because they use it to report the "process" event. Thus, we can't rely on
Expand All @@ -304,15 +316,18 @@ def start_debugging(argv_0):

debugpy.configure(options.config)

if options.mode == "listen" and options.address is not None:
debugpy.listen(options.address)
elif options.mode == "connect" and options.address is not None:
debugpy.connect(options.address, access_token=options.adapter_access_token)
else:
raise AssertionError(repr(options.mode))
if os.environ.get("DEBUGPY_RUNNING", "false") != "true":
Copy link
Contributor

@rchiodo rchiodo Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why you put the flag in the environment variables instead of just a global?

Copy link
Contributor Author

@oelhammouchi oelhammouchi Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I drew inspiration from the werkzeug code which distinguishes subprocesses spawned in the reloader in this way (it was werkzeug's changes that originally caused the problem). I'm not sure a global will work since it won't be set when the child process spawns, or am I wrong?

Copy link
Contributor

@rchiodo rchiodo Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay so this is specifically the case that a subprocess shouldn't connect. I'm wondering if there's a situation that this might break. Like if you wanted to connect to subprocesses too. I believe that's what the subProcess flag is for. Would this change prevent all subProcess connections from working?

Could you instead pass --config-subprocess=false during your initial start of flask? (as described here:

debugpy/README.md

Line 61 in fb6158a

-m debugpy --listen localhost:5678 --pid 12345 --configure-subProcess False
)

Copy link
Contributor

@rchiodo rchiodo Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that doesn't work, I don't think this is the fix we'd want. It seems like the reload from werkzeug should cause the original process to die before reloading. Maybe we can do that here (wait for the original server to finish if we get an address in use).

Copy link
Contributor Author

@oelhammouchi oelhammouchi Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I'm trying to verify this use case but I can't get it working. I'm using the example app

# playground.py
from flask import Flask
from multiprocessing import Process, Queue
from time import sleep

app = Flask(__name__)


def long_calculation(queue: Queue):
    sleep(5)
    queue.put(5)


@app.route("/")
def get_data():
    queue = Queue()
    p = Process(target=long_calculation, args=(queue,))
    p.start()
    p.join()
    res = queue.get()
    return str(res), 200

which I'm running with the command

python -m debugpy --listen 127.0.0.1:5678 --configure-subProcess False -m flask -A playground run --no-debug

However, if I set a breakpoint in the helper function in VS Code and connect, it blocks at the breakpoint without showing this in the UI, so that the only way to unblock things is to shut down the program.

If I'm doing something wrong here, can you indicate how the flag is supposed to work? It's difficult to test if my fix would break this functionality without knowing how it's supposed to work in the first place 😅

Copy link
Contributor Author

@oelhammouchi oelhammouchi Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it doesn't attach to child processes, I wouldn't expect it to hit a breakpoint inside a function running in such a process, but it does.

On Linux it doesn't seem to work with the latest bits, at least for me (as I commented on the issue as well).

Copy link
Contributor

@rchiodo rchiodo Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay I see what you mean about blocking the UI. If I stick a breakpoint in the subprocess code (and subProcess is false), the request never finishes. Without the breakpoint it works fine.

Seems like a separate issue, but yes it doesn't debug a child process if you pass the --configure-subProcess False. Which seems like what you'd want. I'm afraid your change would cause the same thing for all sub processes.

So with your change, can you still hit a breakpoint in the long_calculation?

Copy link
Contributor

@rchiodo rchiodo Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried it and it seems like it works still. It doesn't force subprocess debugging off.

Copy link
Contributor Author

@oelhammouchi oelhammouchi Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can

Copy link
Contributor

@int19h int19h Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still works because subprocesses don't use this code path to connect. Rather, pydevd itself handles subprocesses here, such that the subprocess immediately connects to the debugpy adapter process (which then propagates this new connection to the client).

if options.mode == "listen" and options.address is not None:
debugpy.listen(options.address)
elif options.mode == "connect" and options.address is not None:
debugpy.connect(options.address, access_token=options.adapter_access_token)
else:
raise AssertionError(repr(options.mode))

if options.wait_for_client:
debugpy.wait_for_client()

if options.wait_for_client:
debugpy.wait_for_client()
os.environ["DEBUGPY_RUNNING"] = "true"


def run_file():
Expand Down