Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
046ef86
initial commit
juliusgeo Nov 10, 2022
e042e44
major refactor, I think I have the logic correct this time
juliusgeo Nov 14, 2022
27870fe
remove uneeded stuff
juliusgeo Nov 14, 2022
b32d420
Merge remote-tracking branch 'upstream/master' into PYTHON-3388
juliusgeo Nov 14, 2022
bf0e8db
fix mypy
juliusgeo Nov 14, 2022
76727df
raising the WriteConcernError messes with spec tests
juliusgeo Nov 14, 2022
2120179
major refactor to ensure we are placing the subsequent failpoint corr…
juliusgeo Nov 15, 2022
1747e38
in the middle of a large refactor, saving progress
juliusgeo Nov 15, 2022
f40daec
finish refactor, test passing now
juliusgeo Nov 15, 2022
527c8c3
we need to have NoWritesPerformed label to return original error
juliusgeo Nov 15, 2022
4bcdd21
skip some tests, change to using self.assertSomething, remove todo, f…
juliusgeo Nov 15, 2022
b68ed63
add back in line that was removed
juliusgeo Nov 16, 2022
5f96242
update minversion
juliusgeo Nov 16, 2022
f8122bf
add lock, change logic to match go-driver
juliusgeo Nov 16, 2022
824008b
refactor logic and tests
juliusgeo Nov 17, 2022
31f148d
add assertion to make mypy shut up
juliusgeo Nov 17, 2022
bdd5b46
remove unecessary line
juliusgeo Nov 17, 2022
2800643
fix up whitespace
juliusgeo Nov 17, 2022
b60847d
remove stuff from mongoclient init
juliusgeo Nov 17, 2022
643c109
remove assertion
juliusgeo Nov 17, 2022
98fd502
make sure to save last_error
juliusgeo Nov 17, 2022
23be66e
change back to using only one variable
juliusgeo Nov 17, 2022
67c73fc
pull in latest spec tests
juliusgeo Nov 21, 2022
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
7 changes: 7 additions & 0 deletions pymongo/mongo_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
PyMongoError,
ServerSelectionTimeoutError,
WaitQueueTimeoutError,
WriteConcernError,
)
from pymongo.lock import _HAS_REGISTER_AT_FORK, _create_lock, _release_locks
from pymongo.pool import ConnectionClosedReason
Expand Down Expand Up @@ -836,6 +837,8 @@ def __init__(
# This will be used later if we fork.
MongoClient._clients[self._topology._topology_id] = self

self._indefinite_error: Optional[WriteConcernError] = None
Copy link
Member

Choose a reason for hiding this comment

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

We can't use an instance variable to track this because it makes MongoClient not thread safe.


def _init_background(self):
self._topology = Topology(self._topology_settings)

Expand Down Expand Up @@ -1405,6 +1408,10 @@ def is_retrying():
# Add the RetryableWriteError label, if applicable.
_add_retryable_write_error(exc, max_wire_version)
retryable_error = exc.has_error_label("RetryableWriteError")
if self._indefinite_error and exc.has_error_label("NoWritesPerformed"):
raise self._indefinite_error from exc
if isinstance(exc, WriteConcernError):
self._indefinite_error = exc
Copy link
Member

Choose a reason for hiding this comment

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

_indefinite_error is not needed. last_error already tracks the error that should be propagated.

Copy link
Contributor Author

@juliusgeo juliusgeo Nov 16, 2022

Choose a reason for hiding this comment

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

last_error just tracks the last error, it does not track the last WriteConcernError, which is what we want to do. This ticket's purpose is to have different behavior for normal retryable errors compared to indefinite errors like WriteConcernError. Changing the logic to use last_error makes it not raise the error correctly:

  File "/Users/julius/Work/mongo-python-driver/test/test_retryable_writes.py", line 648, in test_returns_original_error_code
    client.test.test.insert_one({"_id": 1})
AssertionError: WriteConcernError not raised

Copy link
Member

Choose a reason for hiding this comment

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

Why do you think we need to track the last WriteConcernError? This change should not track WriteConcernErrors any differently than other errors. This change is about errors labeled with NoWritesPerformed which may or may not be a WriteConcernError.

if retryable_error:
session._unpin()
if not retryable_error or (is_retrying() and not multiple_retries):
Expand Down
71 changes: 70 additions & 1 deletion test/test_retryable_writes.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,11 @@
from test.utils import (
CMAPListener,
DeprecationFilter,
EventListener,
OvertCommandListener,
TestCreator,
rs_or_single_client,
wait_until,
)
from test.utils_spec_runner import SpecRunner
from test.version import Version
Expand All @@ -45,6 +47,7 @@
)
from pymongo.mongo_client import MongoClient
from pymongo.monitoring import (
CommandSucceededEvent,
ConnectionCheckedOutEvent,
ConnectionCheckOutFailedEvent,
ConnectionCheckOutFailedReason,
Expand All @@ -64,6 +67,24 @@
_TEST_PATH = os.path.join(os.path.dirname(os.path.realpath(__file__)), "retryable_writes", "legacy")


class InsertEventListener(EventListener):
def __init__(self):
self._retried = False
super().__init__()

def reset(self) -> None:
self._retried = False
super().reset()

def succeeded(self, event: CommandSucceededEvent):
if (
event.command_name == "insert"
and event.reply.get("writeConcernError", {}).get("code", None) == 91
):
self._retried = True
super(InsertEventListener, self).succeeded(event)


class TestAllScenarios(SpecRunner):
RUN_ON_LOAD_BALANCER = True
RUN_ON_SERVERLESS = True
Expand Down Expand Up @@ -581,8 +602,56 @@ def test_pool_paused_error_is_retryable(self):
failed = cmd_listener.failed_events
self.assertEqual(1, len(failed), msg)

@client_context.require_failCommand_fail_point
@client_context.require_replica_set
@client_knobs(heartbeat_frequency=0.05, min_heartbeat_interval=0.05)
def test_returns_original_error_code(
self,
): # TODO: Make this a real integration test where we stepdown the primary.
cmd_listener = InsertEventListener()
client = rs_or_single_client(retryWrites=True, event_listeners=[cmd_listener])
client.test.test.drop()
client.test.another_coll.drop()
self.addCleanup(client.close)
cmd_listener.reset()
client.admin.command(
{
"configureFailPoint": "failCommand",
"mode": {"times": 1},
"data": {
"writeConcernError": {
"code": 91,
"errorLabels": ["RetryableWriteError"],
},
"failCommands": ["insert"],
},
}
)
client.test.test.insert_one({"_id": 1})
wait_until(lambda: cmd_listener._retried, "ready for second failpoint")
client.test.test.drop()
client.admin.command(
{
"configureFailPoint": "failCommand",
"mode": {"times": 1},
"data": {
"errorCode": 10107,
"errorLabels": ["RetryableWriteError", "NoWritesPerformed"],
"failCommands": ["insert"],
},
}
)
with self.assertRaises(WriteConcernError) as exc:
client.test.test.insert_one({"_id": 1})
assert exc.exception.code == 91
client.admin.command(
{
"configureFailPoint": "failCommand",
"mode": "off",
}
)


# TODO: Make this a real integration test where we stepdown the primary.
class TestRetryableWritesTxnNumber(IgnoreDeprecationsTest):
@client_context.require_replica_set
@client_context.require_no_mmap
Expand Down