Return the endpoint from listen() with the in-process adapter - #2051
Conversation
debugpy.listen(..., in_process_debug_adapter=True) returned None, while the out-of-process path and the documented API both return the (host, port) tuple. That's a problem when you pass port 0, since there's then no way to find out which port was actually chosen. Return the endpoint and set listen.called so the in-process path matches the out-of-process one. Fixes microsoft#1656. Signed-off-by: Karan Dhaodiyal <256503836+karandhaodiyal28-hash@users.noreply.github.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
| ) | ||
| return | ||
| listen.called = True | ||
| return host, port |
There was a problem hiding this comment.
Issue · Please address or respond
port is still the requested input value here, so listen(0, in_process_debug_adapter=True) now returns (host, 0) rather than the OS-assigned port. This does not meet the documented endpoint contract or the PR's stated port-0 motivation. Please obtain and return pydevd's bound port, or narrow the change and its description to explicitly document this limitation.
[verified]
| # Regression test for #1656: listen(..., in_process_debug_adapter=True) used | ||
| # to return None, unlike the out-of-process path which returns (host, port). | ||
| endpoint = _api.listen(("127.0.0.1", 5678), in_process_debug_adapter=True) | ||
| assert endpoint == ("127.0.0.1", 5678) |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
The fixed-port, no-op _settrace test only verifies that the input endpoint is echoed back; it cannot catch the port=0 contract gap. Add coverage that establishes whether the in-process path returns the resolved bound port for port 0.
[verified]
Rich Chiodo (rchiodo)
added
the
review-auto:changes-requested
|
Updated! Retrieved the actual bound port from pydevd for the in-process adapter path, and added regression test coverage for port 0. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| pydb.wait_for_server_socket_ready() | ||
| actual_host, actual_port = pydb._server_socket_name | ||
| else: | ||
| actual_host, actual_port = host, port |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
If _settrace() succeeds without installing a global debugger, this fallback returns the requested endpoint and can silently return port 0, violating the documented actual-endpoint contract. Treat the missing debugger as an invariant failure rather than returning a success-shaped value.
[verified]
| def no_settrace(monkeypatch): | ||
| # Avoid actually starting pydevd; we only care about listen()'s return value. | ||
| monkeypatch.setattr(_api, "_settrace", lambda **kwargs: None) | ||
| monkeypatch.setattr(_api.listen, "called", False) |
There was a problem hiding this comment.
Info · Optional note
The fixture stubs _settrace but not ensure_logging(), which the @_starts_debugging wrapper still calls and can leave process-global logging state behind for other tests. Stub or restore that state so this unit test remains isolated.
[verified]
Rich Chiodo (rchiodo)
left a comment
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Rich Chiodo (rchiodo)
added
review-auto:approved
Stella Huang (StellaHuang95)
left a comment
Stella Huang (StellaHuang95)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
debugpy.listen()is documented to return the(host, port)it ends up listening on, and it does — except when you passin_process_debug_adapter=True, where it returnsNone:The out-of-process path returns
client_host, client_portat the end, but the in-process branch just did a barereturn. This is especially annoying when you pass port0, because there's then no way to find out which port was actually chosen.This returns the endpoint from the in-process branch as well, and sets
listen.calledso both branches behave the same way with respect to the "listen already called" guard.I added a small regression test that stubs out
_settraceand checks the return value.Fixes #1656.