Properly update pydevd._settrace.called #1751

lukejriddle · 2024-12-03T15:09:33Z

api._settrace.called is intended to confirm that a process had been traced before. However, returning in a try block prevents the subsequent else block from executing, so api._settrace was not updating this flag.

This change just removes the return. api._settrace will still return None, as pydevd.settrace was returning None before.

rchiodo · 2024-12-03T17:07:36Z

/azp run

azure-pipelines · 2024-12-03T17:07:47Z

Azure Pipelines successfully started running 1 pipeline(s).

rchiodo

It looks like this is causing attach tests to fail on Linux. Not sure why, but if you follow the directions in the CONTRIBUTING.md, you should be able to run the failing tests yourself.

lukejriddle · 2024-12-03T20:04:19Z

Looking into the test failures now, but I'm not able to run them locally (on Linux). I get the error Could not find .so for attach to process.. Any ideas why? Seems like debugpy isn't building the attach .so when running tox.

Repro:

Check out repo
Make new python venv and pip install flake8 black tox
Run python -m tox

rchiodo · 2024-12-03T20:10:26Z

Oh we changed the repository to not have the binaries checked in. You have to build pydevd first. Forgot about that. I should update the contributing.md.

Basically:

cd src/debugpy/_vendored/pydevd
Set PYTHONPATH=.
Run python build_tools/build.py

lukejriddle · 2024-12-03T20:57:50Z

Still not able to get the tests working. That build script didn't seem to build the attach .so, so I manually ran pydevd_attach_to_process/linux_and_mac/compile.sh as well. This fixed the error of not finding the shared library, but now I get an opaque error about gdb having an exit code of 1.

rchiodo · 2024-12-03T21:19:04Z

This fixed the error of not finding the shared library, but now I get an opaque error about gdb having an exit code of 1.

That's likely the attach failing. You'll have to add extra logging to the attach code I'm guessing.

rchiodo · 2024-12-03T21:20:44Z

Debugging tests failures is all looking at log files. If there's not enough information, you add more logging. Attach happens in this code here:

debugpy/src/debugpy/_vendored/pydevd/pydevd_attach_to_process/linux_and_mac/attach.cpp

Line 56 in a78e5c2

int DoAttach(bool isDebug, const char *command, bool showDebugInfo)

You'd add more printfs and then rebuild the .so files again.

lukejriddle · 2024-12-04T17:51:00Z

Okay, figured out why the tests are failing. In the test, multiple adapters are attached to the same PID as session1 and session2. Before our change, the check for _settrace.called would never pass, allowing this behavior. Now, attaching multiple adapters to the same PID is essentially forbidden.

This is likely how it should work, though, and has even been requested in a previous issue, since api.listen should only be called once-per-process.

I can update the test to expect this failure, if that works with you @rchiodo

rchiodo · 2024-12-04T17:58:53Z

I can update the test to expect this failure, if that works with you @rchiodo

I believe the test is trying to reattach. We definitely want that to work. Just allowing the failure would be incorrect. Maybe detach has to set the trace flag to false and then the test will pass.

lukejriddle · 2024-12-05T15:59:40Z

@microsoft-github-policy-service agree company="Meta"

lukejriddle · 2024-12-05T16:01:31Z

Okay, how about this? Changed from _settrace.called to _listen.called, since its the multiple invocations of listen that will cause the debuggee to hang; multiple calls to settrace don't, as demonstrated by the attach tests.

rchiodo · 2024-12-05T16:08:59Z

/azp run

azure-pipelines · 2024-12-05T16:09:09Z

Azure Pipelines successfully started running 1 pipeline(s).

rchiodo · 2024-12-05T16:14:42Z

I think you need to run ruff? The linter is failing.

Not sure about using listen instead. Sounds plausible, but the old settrace check also made sure that connect wasn't called more than once. Although maybe connect being called more than once is fine-ish? Connect while you're actually debugging and then reconnect after disconnect should be okay, but connect and then connect again isnt.

But the original code was letting that happen anyway so maybe it's fine.

lukejriddle · 2024-12-05T16:23:19Z

Whoops, did a typo, just pushed the fix as well as the lint issues.

As a follow up, I can look into preventing additional connect calls to the same process if the debuggee already has an adapter connected. Otherwise behavior is pretty much unchanged other than preventing listen from hanging a process

--
Btw, I'm not sure what's going on with tox/pytest, but there are some tests (like the ones in test_attach.py) that pass when run in series using -n0 but fail if taken by a worker. Not sure if it's a config issue on my part

rchiodo · 2024-12-05T16:52:20Z

Btw, I'm not sure what's going on with tox/pytest, but there are some tests (like the ones in test_attach.py) that pass when run in series using -n0 but fail if taken by a worker. Not sure if it's a config issue on my part

Pytest doesn't play well with the way debugpy is running. Timeouts and such are killing processes before they're done. Mostly only happens on windows though.

At least for me. But if the test passes with -n0, then it's safe to say you fixed something.

rchiodo · 2024-12-05T16:52:45Z

/azp run

azure-pipelines · 2024-12-05T16:52:56Z

Azure Pipelines successfully started running 1 pipeline(s).

lukejriddle · 2024-12-05T18:16:54Z

Not sure about these test failures... test_attach_pid_client[str] passes but [int] fails. And only in some environments...: AttributeError: 'GdbRemoveReadlineFinder' object has no attribute 'find_spec'

rchiodo · 2024-12-05T18:24:37Z

Most of the tests are failing because of the new test:

             Traceback (most recent call last):
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\server\api.py", line 130, in debug
                 return func(address, settrace_kwargs, **kwargs)
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\server\api.py", line 144, in listen
                 raise RuntimeError("debugpy.listen() has already been called on this process")
             RuntimeError: debugpy.listen() has already been called on this process
             
             Stack where logged:
               File "C:\Users\VssAdministrator\AppData\Local\Temp\pytest-of-unknown\pytest-0\popen-gw2\test_multiple_listen_raises_ex0\code_to_debug.py", line 12, in <module>
                 debugpy.listen(address=(host, port))
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\public_api.py", line 31, in wrapper
                 return wrapped(*args, **kwargs)
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\server\api.py", line 132, in debug
                 log.reraise_exception("{0}() failed:", func.__name__, level="info")
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\common\log.py", line 222, in reraise_exception
                 _exception(format_string, *args, **kwargs)

I don't think it's raising a runtime error.

lukejriddle · 2024-12-05T18:44:03Z

Hmmm, it looks like it is, from those logs. And we also see the expected listen_exception message sent through the backchannel:

I+00001.266: listen() failed:
             
             Traceback (most recent call last):
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\server\api.py", line 130, in debug
                 return func(address, settrace_kwargs, **kwargs)
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\server\api.py", line 144, in listen
                 raise RuntimeError("debugpy.listen() has already been called on this process")
             RuntimeError: debugpy.listen() has already been called on this process
             
             Stack where logged:
               File "C:\Users\VssAdministrator\AppData\Local\Temp\pytest-of-unknown\pytest-0\popen-gw0\test_multiple_listen_raises_ex0\code_to_debug.py", line 12, in <module>
                 debugpy.listen(address=(host, port))
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\public_api.py", line 31, in wrapper
                 return wrapped(*args, **kwargs)
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\server\api.py", line 132, in debug
                 log.reraise_exception("{0}() failed:", func.__name__, level="info")
               File "D:\a\1\s\.tox\py39\lib\site-packages\debugpy\common\log.py", line 222, in reraise_exception
                 _exception(format_string, *args, **kwargs)
             

D+00001.266: backchannel <-- listen_exception

If this backchannel message weren't received we'd see an AssertionError, not worker 'gw5' crashed while running 'tests/debugpy/test_attach.py::test_multiple_listen_raises_exception[program]'

rchiodo · 2024-12-05T18:45:51Z

Oh then it's crashing the debugger, it's not that it isn't sending it.

lukejriddle · 2024-12-05T19:10:12Z

I can't repro these issues on Linux py3.10 or Mac py3.13. Can I add similar skip rules like the other tests in this file? https://github.com/microsoft/debugpy/blob/main/tests/debugpy/test_attach.py#L156

rchiodo · 2024-12-05T19:32:20Z

I can't repro these issues on Linux py3.10 or Mac py3.13. Can I add similar skip rules like the other tests in this file? https://github.com/microsoft/debugpy/blob/main/tests/debugpy/test_attach.py#L156

I think the root cause of those failures is just that pytest is flakey. Next run they'd likely pass. Ideally we'd make all of these things more deterministic.

I'd rather not skip them though cause eventually we end up skipping everything. Maybe a retry?

lukejriddle · 2024-12-05T19:59:41Z

Yeah sounds good. Do I have permissions to retry?

lukejriddle · 2024-12-05T19:59:46Z

/azp run

azure-pipelines · 2024-12-05T19:59:53Z

Commenter does not have sufficient privileges for PR 1751 in repo microsoft/debugpy

rchiodo · 2024-12-05T20:21:48Z

/azp run

azure-pipelines · 2024-12-05T20:21:59Z

Azure Pipelines successfully started running 1 pipeline(s).

rchiodo · 2024-12-05T20:24:39Z

Wait did you change anything with the new double listen test? I think the test has to somehow manage the debugger crashing. I don't see another test doing that but I think it might just work if you put a try except around the code like so:

try:
  with debug.Session() as session:
        session.open_backchannel()
        session.spawn_debuggee(
            [
                code_to_debug,
                host,
                port,
            ]
        )
        session.wait_for_adapter_socket()

        session.expect_server_socket()
        session.connect_to_adapter((host, port))
        with session.request_attach():
            pass
except:
   pass

And then assert that the backchannel got the exception.

lukejriddle · 2024-12-06T18:04:55Z

Was able to repro locally. Updated the test to finish the lifecycle of the adapter, went from failing to passing so should hopefully work in the pipeline

rchiodo · 2024-12-06T18:13:01Z

/azp run

azure-pipelines · 2024-12-06T18:13:11Z

Azure Pipelines successfully started running 1 pipeline(s).

rchiodo · 2024-12-06T22:49:06Z

Thanks for the work :)

Properly update pydevd._settrace.called

dd96ec9

lukejriddle requested a review from a team as a code owner December 3, 2024 15:09

rchiodo approved these changes Dec 3, 2024

View reviewed changes

rchiodo requested changes Dec 3, 2024

View reviewed changes

Change _settrace.called to _listen.called

26a64cd

Luke Riddle added 2 commits December 5, 2024 08:14

Remove unnecessary underscore from listen

830b306

Fix lint issues

cb0ca5e

Update multiple listen test

1551eef

rchiodo approved these changes Dec 6, 2024

View reviewed changes

rchiodo merged commit 5014f25 into microsoft:main Dec 6, 2024
24 checks passed

Properly update pydevd._settrace.called #1751

Properly update pydevd._settrace.called #1751

Uh oh!

Conversation

lukejriddle commented Dec 3, 2024

Uh oh!

rchiodo commented Dec 3, 2024

Uh oh!

azure-pipelines bot commented Dec 3, 2024

Uh oh!

rchiodo left a comment

Choose a reason for hiding this comment

Uh oh!

lukejriddle commented Dec 3, 2024

Uh oh!

rchiodo commented Dec 3, 2024

Uh oh!

lukejriddle commented Dec 3, 2024

Uh oh!

rchiodo commented Dec 3, 2024

Uh oh!

rchiodo commented Dec 3, 2024

Uh oh!

lukejriddle commented Dec 4, 2024

Uh oh!

rchiodo commented Dec 4, 2024

Uh oh!

lukejriddle commented Dec 5, 2024

Uh oh!

lukejriddle commented Dec 5, 2024

Uh oh!

rchiodo commented Dec 5, 2024

Uh oh!

azure-pipelines bot commented Dec 5, 2024

Uh oh!

rchiodo commented Dec 5, 2024

Uh oh!

lukejriddle commented Dec 5, 2024

Uh oh!

rchiodo commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchiodo commented Dec 5, 2024

Uh oh!

azure-pipelines bot commented Dec 5, 2024

Uh oh!

lukejriddle commented Dec 5, 2024

Uh oh!

rchiodo commented Dec 5, 2024

Uh oh!

lukejriddle commented Dec 5, 2024

Uh oh!

rchiodo commented Dec 5, 2024

Uh oh!

lukejriddle commented Dec 5, 2024

Uh oh!

rchiodo commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukejriddle commented Dec 5, 2024

Uh oh!

lukejriddle commented Dec 5, 2024

Uh oh!

azure-pipelines bot commented Dec 5, 2024

Uh oh!

rchiodo commented Dec 5, 2024

Uh oh!

azure-pipelines bot commented Dec 5, 2024

Uh oh!

rchiodo commented Dec 5, 2024

Uh oh!

lukejriddle commented Dec 6, 2024

Uh oh!

rchiodo commented Dec 6, 2024

Uh oh!

azure-pipelines bot commented Dec 6, 2024

Uh oh!

Uh oh!

rchiodo commented Dec 6, 2024

Uh oh!

rchiodo commented Dec 5, 2024 •

edited

Loading

rchiodo commented Dec 5, 2024 •

edited

Loading