-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-2938 Fix race condition caused by MongoClient._process_periodic_tasks(client) #752
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
Conversation
pymongo/mongo_client.py
Outdated
if client._get_topology._closed: | ||
return False | ||
else: | ||
raise e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will actually work. The problem is that _process_periodic_tasks
already catches and logs the InvalidOperation error which is want we want to prevent. We need to refactor this kind of logic so that we can ignore InvalidOperation without logging it at all:
try:
self._cleanup_cursor(True, cursor_id, address, sock_mgr,
None, False)
except Exception:
helpers._handle_exception()
It would also be good to add a test for this. Perhaps something like this would work but it requires some experimentation:
coll.insert_many([{} for _ in range(5])
cursor = coll.find(batchSize=2)
cursor.next()
assert cursor.cursor_id
client.close()
# Add cursor to kill cursors queue
del cursor
wait_until(cursor_id is added to client's kill cursors queue)
client._process_periodic_tasks() # This must not raise or print any exceptions
test/test_client.py
Outdated
client.close() | ||
# Add cursor to kill cursors queue | ||
del cursor | ||
wait_until(lambda: c_id in [c for _, c, _ in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simplify this to lambda: client._MongoClient__kill_cursors_queue
client._process_periodic_tasks() # This must not raise or print any exceptions | ||
except Exception: | ||
self.fail("client._process_periodic_tasks() raised an exception") | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the try/except/fail here. Just call client._process_periodic_tasks(). If that raises unexpectedly the teat will fail automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/test_client.py
Outdated
cursor = coll.find(batch_size=2) | ||
cursor.next() | ||
c_id = cursor.cursor_id | ||
assert c_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use self.assertIsNotNone here.
@@ -1591,6 +1591,25 @@ def test_network_error_message(self): | |||
with self.assertRaisesRegex(AutoReconnect, expected): | |||
client.pymongo_test.test.find_one({}) | |||
|
|||
def test_process_periodic_tasks(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this test will fail on PyPy because garbage collection works slightly differently. Let's skip this test entirely when running with PyPy (see my current PR for how to do that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongo/mongo_client.py
Outdated
@@ -1583,6 +1583,8 @@ def _process_kill_cursors(self): | |||
try: | |||
self._cleanup_cursor(True, cursor_id, address, sock_mgr, | |||
None, False) | |||
except InvalidOperation as e: | |||
raise e | |||
except Exception: | |||
helpers._handle_exception() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another try/except thats needs to be updated below too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still missing (line 1602).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongo/mongo_client.py
Outdated
self._topology.update_pool(self.__all_credentials) | ||
except InvalidOperation: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only want to ignore this error if the client was closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
We need to fix this bug:
|
pymongo/mongo_client.py
Outdated
@@ -1583,6 +1583,11 @@ def _process_kill_cursors(self): | |||
try: | |||
self._cleanup_cursor(True, cursor_id, address, sock_mgr, | |||
None, False) | |||
except InvalidOperation as e: | |||
if self._topology._closed: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth adding a comment here to explain why we're raising the error when the client is closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongo/mongo_client.py
Outdated
@@ -1583,6 +1583,11 @@ def _process_kill_cursors(self): | |||
try: | |||
self._cleanup_cursor(True, cursor_id, address, sock_mgr, | |||
None, False) | |||
except InvalidOperation as e: | |||
if self._topology._closed: | |||
raise e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use bare raise
instead of raise e
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongo/mongo_client.py
Outdated
if self._topology._closed: | ||
pass | ||
else: | ||
helpers._handle_exception() | ||
except Exception: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be cleaner:
except Exception as exc:
if isinstance(exc, InvalidOperation) and self._topology._closed:
return # Ignore when client has been closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pymongo/mongo_client.py
Outdated
@@ -1583,6 +1583,8 @@ def _process_kill_cursors(self): | |||
try: | |||
self._cleanup_cursor(True, cursor_id, address, sock_mgr, | |||
None, False) | |||
except InvalidOperation as e: | |||
raise e | |||
except Exception: | |||
helpers._handle_exception() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still missing (line 1602).
This typo has been fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor comments
pymongo/mongo_client.py
Outdated
@@ -1607,17 +1612,24 @@ def _process_kill_cursors(self): | |||
self._kill_cursors( | |||
cursor_ids, address, topology, session=None) | |||
except Exception: | |||
helpers._handle_exception() | |||
if (isinstance(e,InvalidOperation) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a space between e,
and InvalidOperation.
pymongo/mongo_client.py
Outdated
@@ -1596,8 +1596,13 @@ def _process_kill_cursors(self): | |||
try: | |||
self._cleanup_cursor(True, cursor_id, address, sock_mgr, | |||
None, False) | |||
except Exception: | |||
helpers._handle_exception() | |||
except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename e to exc everywhere. We usually use exc as the name for exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.