-
Notifications
You must be signed in to change notification settings - Fork 146
If the token is null, the connection hangs (#458) #876
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
3cb80da
11579f5
f255f4a
3afd559
7e3e520
8aa4e7d
9ea4bb8
121bd0b
ac27f93
2d6f729
eb1af56
bf48dae
ead14dd
91e698e
fbdb5dc
c10bee7
dcba798
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,12 @@ def refresh(self) -> Token: | |
|
|
||
|
|
||
| class OAuthManager: | ||
| # Maximum time (in seconds) to wait for the browser OAuth redirect callback | ||
| # before giving up. Without this, the local callback server would block | ||
| # forever in a headless environment (e.g. a notebook/job with no browser), | ||
| # making the connection appear to hang indefinitely. See issue #458. | ||
| REDIRECT_CALLBACK_TIMEOUT_SECONDS = 60 * 5 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Medium — This introduces a hard 5-minute cap on every U2M interactive login, not just the headless failure case, and — as the docstring itself notes — there is no public Separately, for the exact reported scenario in #458 (headless notebook/job), the common path is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed the concern is valid, but neither remedy is actionable in this bug-fix PR. (a) Exposing the redirect-callback timeout as a public/documented knob is a public API change — it must be plumbed through DatabricksOAuthProvider and the public connect()/Connection kwargs (the code deliberately keeps it internal-only), which is out of scope here and belongs in a separate API-review PR. (b) Fast-failing when webbrowser.open_new() returns False (the common #458 headless path) conflicts with a deliberate, documented decision (oauth.py ~L222-237) that a falsy return is not a reliable cross-platform headless signal and would break working interactive logins. The remaining question — the correct default ceiling and whether to expose a public override for a widely-consumed connector, given that logins >5 min that previously succeeded will now fail — is a product/API judgment call for a human maintainer, and further bot back-and-forth won't resolve it. Flagging for human review. |
||
|
|
||
| def __init__( | ||
| self, | ||
| port_range: List[int], | ||
|
|
@@ -130,9 +136,24 @@ def __get_authorization_code(self, client, auth_url, scope, state, challenge): | |
| handler = OAuthHttpSingleRequestHandler("Databricks Sql Connector") | ||
|
|
||
| last_error = None | ||
| callback_timed_out = False | ||
| for port in self.port_range: | ||
| try: | ||
| with HTTPServer(("", port), handler) as httpd: | ||
| # Bound how long we wait for the browser redirect callback so | ||
| # that a headless environment (no browser to complete the | ||
| # flow) fails with a clear error instead of hanging forever. | ||
|
peco-review-bot[bot] marked this conversation as resolved.
|
||
| httpd.timeout = self.REDIRECT_CALLBACK_TIMEOUT_SECONDS | ||
|
peco-review-bot[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| # HTTPServer.handle_request() returns normally (via | ||
| # handle_timeout()) when the wait elapses without a | ||
| # connection, so record that case to distinguish it from a | ||
|
peco-review-bot[bot] marked this conversation as resolved.
|
||
| # received-but-empty callback below. | ||
| def _on_timeout(): | ||
| nonlocal callback_timed_out | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — The accept-wait timeout ( 5 minutes is generous and the tradeoff is documented in the class comment, so this is likely acceptable — but note there is no public
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Valid concern, but not actionable in this bug-fix PR — needs a human/maintainer decision. The tradeoff is already documented in the OAuthManager class docstring (oauth.py:63–76): the 5-min ceiling is deliberate and |
||
| callback_timed_out = True | ||
|
|
||
| httpd.handle_timeout = _on_timeout | ||
|
peco-review-bot[bot] marked this conversation as resolved.
|
||
| redirect_url = OAuthManager.__get_redirect_url(port) | ||
| auth_req_uri, _, _ = client.prepare_authorization_request( | ||
| authorization_url=auth_url, | ||
|
|
@@ -164,7 +185,16 @@ def __get_authorization_code(self, client, auth_url, scope, state, challenge): | |
| raise last_error | ||
|
|
||
| if not handler.request_path: | ||
| msg = f"No path parameters were returned to the callback at {redirect_url}" | ||
| if callback_timed_out: | ||
| msg = ( | ||
| f"Timed out after {self.REDIRECT_CALLBACK_TIMEOUT_SECONDS} " | ||
| f"seconds waiting for the OAuth redirect callback at " | ||
| f"{redirect_url}. No browser completed the login flow — this " | ||
| "is expected in a headless environment (e.g. a notebook or " | ||
| "job with no browser). See issue #458." | ||
| ) | ||
| else: | ||
| msg = f"No path parameters were returned to the callback at {redirect_url}" | ||
| logger.error(msg) | ||
| raise RuntimeError(msg) | ||
| # This is a kludge because the parsing library expects https callbacks | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.