Skip to content

[py] Add args to is_url_connectable #16212

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

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

cgoldberg
Copy link
Contributor

@cgoldberg cgoldberg commented Aug 19, 2025

User description

🔗 Related Issues

Fixes #6046
Fixes #3904

💥 What does this PR do?

This PR adds host and scheme args to the is_url_connectable function in the utils module.

I can't find any references to this function in our codebase, but maybe somebody is using it externally? or maybe it's just leftover legacy code? Anyway, it seemed kind of crazy to have a utility method for checking URL connectivity where you can't specify a URL. This should make it more flexible.

🔧 Implementation Notes

The host and scheme args are optional keyword args with default values identical to the old hardcoded values, so this shouldn't break anybody's code that may be using this function.

I also included some minor cleanup of docstrings.

🔄 Types of changes

  • Bug fix (backwards compatible)
  • New feature

PR Type

Enhancement


Description

  • Add host and scheme parameters to is_url_connectable function

  • Improve docstring formatting and clarity across utility functions

  • Maintain backward compatibility with optional parameters


Diagram Walkthrough

flowchart LR
  A["is_url_connectable function"] --> B["Add host parameter"]
  A --> C["Add scheme parameter"]
  B --> D["Enhanced URL connectivity testing"]
  C --> D
  E["Docstring improvements"] --> F["Better documentation"]
Loading

File Walkthrough

Relevant files
Enhancement
utils.py
Enhanced URL connectivity utility with configurable parameters

py/selenium/webdriver/common/utils.py

  • Add host and scheme optional parameters to is_url_connectable function
  • Update function docstring with clearer parameter descriptions
  • Clean up docstrings for other utility functions
  • Import urllib module for URL handling
+20/-13 

@selenium-ci selenium-ci added the C-py Python Bindings label Aug 19, 2025
Copy link
Contributor

qodo-merge-pro bot commented Aug 19, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Support IPv6 host literals

Safely handle IPv6 hosts by wrapping them in brackets when building the URL.
Without this, URLs like IPv6 literals will be invalid and requests will fail.

py/selenium/webdriver/common/utils.py [136-153]

 def is_url_connectable(
     port: Union[int, str],
     host: Optional[str] = "127.0.0.1",
     scheme: Optional[str] = "http",
 ) -> bool:
-    ...
     try:
-        res = urllib.request.urlopen(f"{scheme}://{host}:{port}/status")
+        normalized_scheme = (scheme or "http").lower()
+        if normalized_scheme not in ("http", "https"):
+            return False
+        host_part = f"[{host}]" if host and ":" in host and not host.startswith("[") else host
+        res = urllib.request.urlopen(f"{normalized_scheme}://{host_part}:{port}/status", timeout=1)
         return res.getcode() == 200
     except Exception:
         return False

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies that IPv6 literal addresses need to be wrapped in brackets in a URL, fixing a bug for IPv6 hosts and aligning with the behavior of join_host_port in the same file.

Medium
Add a request timeout

Set a short timeout on the request to avoid indefinite blocking when the host is
unreachable or filtered by network rules. This prevents hangs in environments
with strict firewalls and aligns with the quick check intent of connectivity
functions.

py/selenium/webdriver/common/utils.py [136-153]

 def is_url_connectable(
     port: Union[int, str],
     host: Optional[str] = "127.0.0.1",
     scheme: Optional[str] = "http",
 ) -> bool:
     """Sends a request to the HTTP server at the /status endpoint to see if it
     responds successfully.
 
     :Args:
         - port - port number
         - host - hostname or IP
         - scheme - URL scheme
     """
     try:
-        res = urllib.request.urlopen(f"{scheme}://{host}:{port}/status")
+        res = urllib.request.urlopen(f"{scheme}://{host}:{port}/status", timeout=1)
         return res.getcode() == 200
     except Exception:
         return False
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good suggestion as urllib.request.urlopen can block indefinitely without a timeout, and adding one aligns with the behavior of the similar is_connectable function, improving robustness.

Medium
Security
Validate and normalize scheme

Normalize and validate the scheme to avoid malformed URLs and accidental
injection. Restrict to expected schemes (http, https) and lower-case it before
use to prevent surprises.

py/selenium/webdriver/common/utils.py [136-153]

 def is_url_connectable(
     port: Union[int, str],
     host: Optional[str] = "127.0.0.1",
     scheme: Optional[str] = "http",
 ) -> bool:
     """Sends a request to the HTTP server at the /status endpoint to see if it
     responds successfully.
 
     :Args:
         - port - port number
         - host - hostname or IP
         - scheme - URL scheme
     """
     try:
-        res = urllib.request.urlopen(f"{scheme}://{host}:{port}/status")
+        normalized_scheme = (scheme or "http").lower()
+        if normalized_scheme not in ("http", "https"):
+            return False
+        res = urllib.request.urlopen(f"{normalized_scheme}://{host}:{port}/status", timeout=1)
         return res.getcode() == 200
     except Exception:
         return False
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: Validating the scheme parameter to only allow http and https is a good practice for robustness and security, preventing the use of unintended URL schemes.

Low
Learned
best practice
Close HTTP response via context manager
Suggestion Impact:The code was updated to wrap urlopen in a with-statement, ensuring the response is closed, matching the suggestion.

code diff:

@@ -147,8 +147,8 @@
         - scheme - URL scheme
     """
     try:
-        res = urllib.request.urlopen(f"{scheme}://{host}:{port}/status")
-        return res.getcode() == 200
+        with urllib.request.urlopen(f"{scheme}://{host}:{port}/status") as res:
+            return res.getcode() == 200
     except Exception:

Ensure the HTTP response object is properly closed by using a context manager.
This prevents potential resource leaks under heavy usage or exceptions.

py/selenium/webdriver/common/utils.py [150-151]

-res = urllib.request.urlopen(f"{scheme}://{host}:{port}/status")
-return res.getcode() == 200
+with urllib.request.urlopen(f"{scheme}://{host}:{port}/status") as res:
+    return res.getcode() == 200

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Use proper resource disposal patterns with context managers to prevent leaks when working with network/IO resources.

Low
  • Update

@SeleniumHQ SeleniumHQ deleted a comment from qodo-merge-pro bot Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

py, is_url_connectable check for port , but just localhost url utils.is_url_connectable should take a URL?
2 participants