- Notifications
You must be signed in to change notification settings - Fork 182
Properly update pydevd._settrace.called#1751
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
rchiodo commented Dec 3, 2024
/azp run |
| Azure Pipelines successfully started running 1 pipeline(s). |
rchiodo left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 commented Dec 3, 2024
Looking into the test failures now, but I'm not able to run them locally (on Linux). I get the error Repro:
|
rchiodo commented Dec 3, 2024
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:
|
lukejriddle commented Dec 3, 2024
Still not able to get the tests working. That build script didn't seem to build the attach .so, so I manually ran |
rchiodo commented Dec 3, 2024
That's likely the attach failing. You'll have to add extra logging to the attach code I'm guessing. |
rchiodo commented Dec 3, 2024
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
You'd add more printfs and then rebuild the .so files again. |
lukejriddle commented Dec 4, 2024
Okay, figured out why the tests are failing. In the test, multiple adapters are attached to the same PID as This is likely how it should work, though, and has even been requested in a previous issue, since I can update the test to expect this failure, if that works with you @rchiodo |
rchiodo commented Dec 4, 2024
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 commented Dec 5, 2024
@microsoft-github-policy-service agree company="Meta" |
lukejriddle commented Dec 5, 2024
Okay, how about this? Changed from |
rchiodo commented Dec 5, 2024
/azp run |
| Azure Pipelines successfully started running 1 pipeline(s). |
rchiodo commented Dec 5, 2024
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 commented Dec 5, 2024
Whoops, did a typo, just pushed the fix as well as the lint issues. As a follow up, I can look into preventing additional -- |
rchiodo commented Dec 5, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 commented Dec 5, 2024
/azp run |
| Azure Pipelines successfully started running 1 pipeline(s). |
lukejriddle commented Dec 5, 2024
Not sure about these test failures... |
rchiodo commented Dec 5, 2024
Most of the tests are failing because of the new test: I don't think it's raising a runtime error. |
lukejriddle commented Dec 5, 2024
Hmmm, it looks like it is, from those logs. And we also see the expected If this backchannel message weren't received we'd see an AssertionError, not |
rchiodo commented Dec 5, 2024
Oh then it's crashing the debugger, it's not that it isn't sending it. |
lukejriddle commented Dec 5, 2024
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 commented Dec 5, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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 commented Dec 5, 2024
Yeah sounds good. Do I have permissions to retry? |
lukejriddle commented Dec 5, 2024
/azp run |
| Commenter does not have sufficient privileges for PR 1751 in repo microsoft/debugpy |
rchiodo commented Dec 5, 2024
/azp run |
| Azure Pipelines successfully started running 1 pipeline(s). |
rchiodo commented Dec 5, 2024
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: withdebug.Session() assession: 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)) withsession.request_attach(): passexcept: passAnd then assert that the backchannel got the exception. |
lukejriddle commented Dec 6, 2024
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 commented Dec 6, 2024
/azp run |
| Azure Pipelines successfully started running 1 pipeline(s). |
5014f25 into microsoft:mainUh oh!
There was an error while loading. Please reload this page.
rchiodo commented Dec 6, 2024
Thanks for the work :) |
api._settrace.calledis intended to confirm that a process had been traced before. However, returning in atryblock prevents the subsequentelseblock from executing, soapi._settracewas not updating this flag.This change just removes the return.
api._settracewill still returnNone, aspydevd.settracewas returningNonebefore.