Catch ValueError while waiting for server to be reachable #4733
Conversation
jupyterhub/utils.py
Outdated
| @@ -295,6 +295,8 @@ async def is_reachable(): | |||
| errno.ECONNRESET, | |||
| }: | |||
| app_log.warning("Failed to connect to %s (%s)", url, e) | |||
| except ValueError as e: | |||
There was a problem hiding this comment.
We keep expanding this, I wonder if this should be except Exception as e. That would swallow some errors inappropriately, e.g. NameError: url if there was a typo there, but I think the vast majority of errors to be raised here are likely to be like the one you just encountered. And the failure mode of catching too many here is only a delay in reporting real errors, rather than the current situation which is inappropriate failure that can't be worked around.
What do you think?
There was a problem hiding this comment.
If one can trust the Spawner.poll function, errors and non-running notebook-servers should be catched anyway, shouldn't they?
IMHO we can change it to except Exception as e.
I can update the PR if you like
There was a problem hiding this comment.
Yeah, go for it. I think that makes the most sense.
| errno.ECONNREFUSED, | ||
| errno.ECONNRESET, | ||
| }: | ||
| app_log.warning("Failed to connect to %s (%s)", url, e) |
There was a problem hiding this comment.
Sorry, I wasn't clear what except Exception should replace. This OSError block should stay. We still want to avoid logging the expected "not there yet" errors, but log and not fail on unexpected errors.
There was a problem hiding this comment.
Ah sure. Sorry for that. Should be fine now.
This reverts commit 6bc3e05.
…g OSError specific catch
|
Thanks! |
|
@meeseeksdev please backport to 4.x |
…r to be reachable
…3-on-4.x Backport PR #4733 on branch 4.x (Catch ValueError while waiting for server to be reachable)
closes #4732
This is the patch we've added to our JupyterHub setup, to avoid the failed start attempts described in the issue.