Skip to content

[bug-fix] Return empty list if server_descriptions do not exist - #1361

Closed
piyiotisk wants to merge 1 commit into
mongodb:masterfrom
piyiotisk:master
Closed

[bug-fix] Return empty list if server_descriptions do not exist#1361
piyiotisk wants to merge 1 commit into
mongodb:masterfrom
piyiotisk:master

Conversation

@piyiotisk

@piyiotisk piyiotisk commented Aug 23, 2023 •
edited
Loading

Copy link
Copy Markdown

This PR fixes the following error. This is a part of our CI pipeline (withplum.com)
Unfortunately, almost in every build we encounter this failure. Locally it's difficult to reproduce.
Here's the Jira ticket from Mongodb https://jira.mongodb.org/browse/PYTHON-1271

.venv/lib/python3.9/site-packages/pymongo/database.py:959: in list_collection_names
    return [result["name"] for result in self.list_collections(session=session, **kwargs)]
.venv/lib/python3.9/site-packages/pymongo/database.py:911: in list_collections
    return self.__client._retryable_read(_cmd, read_pref, session)
.venv/lib/python3.9/site-packages/pymongo/_csot.py:105: in csot_wrapper
    return func(self, *args, **kwargs)
.venv/lib/python3.9/site-packages/pymongo/mongo_client.py:1441: in _retryable_read
    server = self._select_server(read_pref, session, address=address)
.venv/lib/python3.9/site-packages/pymongo/mongo_client.py:1257: in _select_server
    server = topology.select_server(server_selector)
.venv/lib/python3.9/site-packages/pymongo/topology.py:272: in select_server
    server = self._select_server(selector, server_selection_timeout, address)
.venv/lib/python3.9/site-packages/pymongo/topology.py:261: in _select_server
    servers = self.select_servers(selector, server_selection_timeout, address)
.venv/lib/python3.9/site-packages/pymongo/topology.py:223: in select_servers
    server_descriptions = self._select_servers_loop(selector, server_timeout, address)
.venv/lib/python3.9/site-packages/pymongo/topology.py:231: in _select_servers_loop
    server_descriptions = self._description.apply_selector(
.venv/lib/python3.9/site-packages/pymongo/topology_description.py:317: in apply_selector
    return self._apply_local_threshold(selection)
.venv/lib/python3.9/site-packages/pymongo/topology_description.py:261: in _apply_local_threshold
    return [
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

.0 = <list_iterator object at 0x7f562981f5b0>

    return [
>       s for s in selection.server_descriptions if (s.round_trip_time - fastest) <= threshold
    ]
E   TypeError: unsupported operand type(s) for -: 'NoneType' and 'NoneType'

.venv/lib/python3.9/site-packages/pymongo/topology_description.py:262: TypeError

@piyiotisk
piyiotisk requested a review from a team as a code owner August 23, 2023 09:28
@piyiotisk
piyiotisk requested review from NoahStapp and removed request for a team August 23, 2023 09:29
KonstantinosTsitoumis
KonstantinosTsitoumis reviewed Aug 23, 2023
View reviewed changes
Comment thread pymongo/topology_description.py
Comment on lines +271 to +272
server_descriptions = selection.server_descriptions
server_descriptions = [s for s in server_descriptions if s.round_trip_time != None]

@KonstantinosTsitoumis KonstantinosTsitoumis Aug 23, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we need to re-assign here? Shouldn't we do it in a one-liner?

stavros47
stavros47 reviewed Aug 23, 2023
View reviewed changes
Comment thread pymongo/topology_description.py
Comment on lines +271 to +274
server_descriptions = selection.server_descriptions
server_descriptions = [s for s in server_descriptions if s.round_trip_time != None]
if not server_descriptions:
return []

@stavros47 stavros47 Aug 23, 2023 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
server_descriptions = selection.server_descriptions
server_descriptions = [s for s in server_descriptions if s.round_trip_time != None]
if not server_descriptions:
return []
server_descriptions = [s for s in (selection.server_descriptions or []) if s.round_trip_time is not None]
if not server_descriptions:
return []

expobrain
expobrain suggested changes Aug 23, 2023
View reviewed changes

@expobrain expobrain left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We have already the patched version in the branch https://github.com/withplum/mongo-python-driver/tree/4.5.0-patched

@ShaneHarvey

ShaneHarvey commented Sep 12, 2023

Copy link
Copy Markdown
Contributor

Hello, thanks for the PR. As stated in PYTHON-1271 it's unexpected for this case to happen since at this point all the servers should be in a known state with a non-None value for round_trip_time. Are you able to reproduce this issue? If so, it would be better for us to debug this case to understand the root cause.

@ShaneHarvey

ShaneHarvey commented Nov 10, 2023

Copy link
Copy Markdown
Contributor

@piyiotisk, @KonstantinosTsitoumis, @stavros47, @expobrain Are you able to reproduce this issue? If so, it would be better for us to debug this case to understand the root cause.

@ShaneHarvey
ShaneHarvey requested review from ShaneHarvey and removed request for NoahStapp November 17, 2023 19:05
@blink1073

blink1073 commented Nov 17, 2023

Copy link
Copy Markdown
Member

We're going to close this pull request for now, please comment if you'd like us to open it back up.

@blink1073 blink1073 closed this Nov 17, 2023
@Ale-Cas

Ale-Cas commented Mar 26, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hi, i'm still able to reproduce this issue with pymongo v4.6.1.

The root cause of the error is probably due to our configuration of the topology / replica set but it would be helpful if this was handled in a more specific way, instead of getting a generic TypeError when computing this list comprehension.

I think the solution proposed here was to basically return an empty list in such cases, but I'm not sure wether that's the best way to handle this, and I would prefer to have a ConfigurationError specifying what's wrong in the configuration in this case.

@blink1073 @ShaneHarvey would you consider re-opening this issue?

@Ale-Cas Ale-Cas mentioned this pull request Mar 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShaneHarvey ShaneHarvey Awaiting requested review from ShaneHarvey
3 more reviewers
@stavros47 stavros47 stavros47 left review comments
@KonstantinosTsitoumis KonstantinosTsitoumis KonstantinosTsitoumis left review comments
@expobrain expobrain expobrain requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@piyiotisk @ShaneHarvey @blink1073 @Ale-Cas @expobrain @stavros47 @KonstantinosTsitoumis