Skip to content
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

PYTHON-3926 Add more information to connection errors and timeouts #1375

Merged
merged 15 commits into from
Oct 30, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions pymongo/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@
from pymongo.write_concern import DEFAULT_WRITE_CONCERN, WriteConcern, validate_boolean

if TYPE_CHECKING:
from pymongo import MongoClient
from pymongo.client_session import ClientSession


ORDERED_TYPES: Sequence[Type] = (SON, OrderedDict)

# Defaults until we connect to a server and get updated limits.
Expand Down Expand Up @@ -793,7 +793,6 @@ def validate_datetime_conversion(option: Any, value: Any) -> Optional[DatetimeCo
"waitqueuetimeoutms",
]


_AUTH_OPTIONS = frozenset(["authmechanismproperties"])


Expand Down Expand Up @@ -871,6 +870,37 @@ def _ecoc_coll_name(encrypted_fields: Mapping[str, Any], name: str) -> Any:
WRITE_CONCERN_OPTIONS = frozenset(["w", "wtimeout", "wtimeoutms", "fsync", "j", "journal"])


def _get_timeout_details(client: MongoClient) -> dict[str, Any]:
details = {}
timeout = client.options.timeout
socket_timeout = client.options.pool_options.socket_timeout
connect_timeout = client.options.pool_options.connect_timeout
topology = client.topology_description.server_descriptions()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be client.topology_description which is what we include in ServerSelectionTimeoutError:

f"{self._error_message(selector)}, Timeout: {timeout}s, Topology Description: {self.description!r}"

if timeout:
details["timeout"] = timeout * 1000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to add _csot.get_timeout() here? That would reflect the last CSOT timeout that was set.

if socket_timeout:
details["socketTimeout"] = socket_timeout * 1000
if connect_timeout:
details["connectTimeout"] = connect_timeout * 1000
if topology:
details["topology"] = topology
return details


def format_timeout_details(details: dict[str, Any]) -> str:
result = ""
if details:
result += " (configured timeouts:"
for timeout in ["socketTimeout", "timeout", "connectTimeout"]:
if timeout in details:
result += f" {timeout}: {details[timeout]}ms,"
result = result[:-1]
result += ")"
if "topology" in details:
result += f"\nServer topology: {details['topology']}"
return result


class BaseObject:
"""A base class that provides attributes and methods common
to multiple pymongo classes.
Expand Down
23 changes: 18 additions & 5 deletions pymongo/pool.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@
MIN_POOL_SIZE,
ORDERED_TYPES,
WAIT_QUEUE_TIMEOUT,
_get_timeout_details,
format_timeout_details,
)
from pymongo.errors import (
AutoReconnect,
Expand Down Expand Up @@ -379,7 +381,10 @@ def _truncate_metadata(metadata: MutableMapping[str, Any]) -> None:


def _raise_connection_failure(
address: Any, error: Exception, msg_prefix: Optional[str] = None
address: Any,
error: Exception,
msg_prefix: Optional[str] = None,
details: Optional[dict[str, Any]] = None,
) -> NoReturn:
"""Convert a socket.error to ConnectionFailure and raise it."""
host, port = address
Expand All @@ -391,13 +396,14 @@ def _raise_connection_failure(
if msg_prefix:
msg = msg_prefix + msg
if isinstance(error, socket.timeout):
raise NetworkTimeout(msg) from error
print(f"DETAILS: {details}")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errant print here.

raise NetworkTimeout(msg + format_timeout_details(details)) from error
elif isinstance(error, SSLError) and "timed out" in str(error):
# Eventlet does not distinguish TLS network timeouts from other
# SSLErrors (https://github.com/eventlet/eventlet/issues/692).
# Luckily, we can work around this limitation because the phrase
# 'timed out' appears in all the timeout related SSLErrors raised.
raise NetworkTimeout(msg) from error
raise NetworkTimeout(msg + format_timeout_details(details)) from error
else:
raise AutoReconnect(msg) from error

Expand Down Expand Up @@ -738,8 +744,12 @@ def apply_timeout(
if max_time_ms < 0:
# CSOT: raise an error without running the command since we know it will time out.
errmsg = f"operation would exceed time limit, remaining timeout:{timeout:.5f} <= network round trip time:{rtt:.5f}"
timeout_details = _get_timeout_details(client)
raise ExecutionTimeout(
errmsg, 50, {"ok": 0, "errmsg": errmsg, "code": 50}, self.max_wire_version
errmsg + format_timeout_details(timeout_details),
50,
{"ok": 0, "errmsg": errmsg, "code": 50},
self.max_wire_version,
)
if cmd is not None:
cmd["maxTimeMS"] = int(max_time_ms * 1000)
Expand Down Expand Up @@ -1549,7 +1559,10 @@ def connect(self, handler: Optional[_MongoClientErrorHandler] = None) -> Connect
)

if isinstance(error, (IOError, OSError, SSLError)):
_raise_connection_failure(self.address, error)
timeout_details = None
if handler:
timeout_details = _get_timeout_details(handler.client)
_raise_connection_failure(self.address, error, details=timeout_details)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm is there a central place we could catch and re-raise these exceptions (rather than sprinkling format_timeout_details in various places)?

Copy link
Contributor Author

@NoahStapp NoahStapp Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we'd be able to access the _MongoClientErrorHandler whenever we catch a timeout exception. Are there existing limitations to our internal network interfaces that prevent us from passing an instance of the handler around more widely? That would allow us to add details to far more timeout errors as well as centralize the logic required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up @ShaneHarvey

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we do already wrap all operations with _MongoClientErrorHandler. Does that answer your question?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_MongoClientErrorHandler is only wrapped around _checkout and _run_operation in MongoClient, are those the only two operations relevant for this issue? If so, we could maybe move the logic in format_timeout_details into the error handler itself to centralize the processing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered in Zoom but summarizing here:
Yes that's true but it's better we add the info at the point the exception is raised to ensure that event publishing and (soon) command logging also include the same info. If we catch and reraise the exception in _MongoClientErrorHandler then the events/logs will see a different error message than the user.


raise

Expand Down
Loading