Skip to content

PYTHON-3388 Propagate Original Error for Write Errors Labeled NoWritesPerformed #1117

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

Merged
merged 23 commits into from
Dec 2, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
10 changes: 8 additions & 2 deletions pymongo/mongo_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -1408,12 +1408,18 @@ def is_retrying():
if retryable_error:
session._unpin()
if not retryable_error or (is_retrying() and not multiple_retries):
raise
if exc.has_error_label("NoWritesPerformed") and last_error:
raise last_error from exc
else:
raise
Copy link
Member

Choose a reason for hiding this comment

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

There's a missing condition here. If the current error is NoWritesPerformed and the last error is None, then we must raise the current error even though it's labelled NoWritesPerformed. This is True for all the cases above with assert last_error is not None too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing the assertion and refactoring fixed this.

if bulk:
bulk.retrying = True
else:
retrying = True
last_error = exc
if not exc.has_error_label("NoWritesPerformed"):
last_error = exc
if last_error is None:
last_error = exc

@_csot.apply
def _retryable_read(self, func, read_pref, session, address=None, retryable=True):
Expand Down
36 changes: 18 additions & 18 deletions test/retryable_writes/unified/handshakeError.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
],
"tests": [
{
"description": "insertOne succeeds after retryable handshake network error",
"description": "collection.insertOne succeeds after retryable handshake network error",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -150,7 +150,7 @@
]
},
{
"description": "insertOne succeeds after retryable handshake server error (ShutdownInProgress)",
"description": "collection.insertOne succeeds after retryable handshake server error (ShutdownInProgress)",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -246,7 +246,7 @@
]
},
{
"description": "insertMany succeeds after retryable handshake network error",
"description": "collection.insertMany succeeds after retryable handshake network error",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -344,7 +344,7 @@
]
},
{
"description": "insertMany succeeds after retryable handshake server error (ShutdownInProgress)",
"description": "collection.insertMany succeeds after retryable handshake server error (ShutdownInProgress)",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -442,7 +442,7 @@
]
},
{
"description": "deleteOne succeeds after retryable handshake network error",
"description": "collection.deleteOne succeeds after retryable handshake network error",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -535,7 +535,7 @@
]
},
{
"description": "deleteOne succeeds after retryable handshake server error (ShutdownInProgress)",
"description": "collection.deleteOne succeeds after retryable handshake server error (ShutdownInProgress)",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -628,7 +628,7 @@
]
},
{
"description": "replaceOne succeeds after retryable handshake network error",
"description": "collection.replaceOne succeeds after retryable handshake network error",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -724,7 +724,7 @@
]
},
{
"description": "replaceOne succeeds after retryable handshake server error (ShutdownInProgress)",
"description": "collection.replaceOne succeeds after retryable handshake server error (ShutdownInProgress)",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -820,7 +820,7 @@
]
},
{
"description": "updateOne succeeds after retryable handshake network error",
"description": "collection.updateOne succeeds after retryable handshake network error",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -918,7 +918,7 @@
]
},
{
"description": "updateOne succeeds after retryable handshake server error (ShutdownInProgress)",
"description": "collection.updateOne succeeds after retryable handshake server error (ShutdownInProgress)",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -1016,7 +1016,7 @@
]
},
{
"description": "findOneAndDelete succeeds after retryable handshake network error",
"description": "collection.findOneAndDelete succeeds after retryable handshake network error",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -1109,7 +1109,7 @@
]
},
{
"description": "findOneAndDelete succeeds after retryable handshake server error (ShutdownInProgress)",
"description": "collection.findOneAndDelete succeeds after retryable handshake server error (ShutdownInProgress)",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -1202,7 +1202,7 @@
]
},
{
"description": "findOneAndReplace succeeds after retryable handshake network error",
"description": "collection.findOneAndReplace succeeds after retryable handshake network error",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -1298,7 +1298,7 @@
]
},
{
"description": "findOneAndReplace succeeds after retryable handshake server error (ShutdownInProgress)",
"description": "collection.findOneAndReplace succeeds after retryable handshake server error (ShutdownInProgress)",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -1394,7 +1394,7 @@
]
},
{
"description": "findOneAndUpdate succeeds after retryable handshake network error",
"description": "collection.findOneAndUpdate succeeds after retryable handshake network error",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -1492,7 +1492,7 @@
]
},
{
"description": "findOneAndUpdate succeeds after retryable handshake server error (ShutdownInProgress)",
"description": "collection.findOneAndUpdate succeeds after retryable handshake server error (ShutdownInProgress)",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -1590,7 +1590,7 @@
]
},
{
"description": "bulkWrite succeeds after retryable handshake network error",
"description": "collection.bulkWrite succeeds after retryable handshake network error",
"operations": [
{
"name": "failPoint",
Expand Down Expand Up @@ -1692,7 +1692,7 @@
]
},
{
"description": "bulkWrite succeeds after retryable handshake server error (ShutdownInProgress)",
"description": "collection.bulkWrite succeeds after retryable handshake server error (ShutdownInProgress)",
"operations": [
{
"name": "failPoint",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
{
"description": "retryable-writes insertOne noWritesPerformedErrors",
"schemaVersion": "1.0",
"runOnRequirements": [
{
"minServerVersion": "6.0",
"topologies": [
"replicaset"
]
}
],
"createEntities": [
{
"client": {
"id": "client0",
"useMultipleMongoses": false,
"observeEvents": [
"commandFailedEvent"
]
}
},
{
"database": {
"id": "database0",
"client": "client0",
"databaseName": "retryable-writes-tests"
}
},
{
"collection": {
"id": "collection0",
"database": "database0",
"collectionName": "no-writes-performed-collection"
}
}
],
"tests": [
{
"description": "InsertOne fails after NoWritesPerformed error",
"operations": [
{
"name": "failPoint",
"object": "testRunner",
"arguments": {
"client": "client0",
"failPoint": {
"configureFailPoint": "failCommand",
"mode": {
"times": 2
},
"data": {
"failCommands": [
"insert"
],
"errorCode": 64,
"errorLabels": [
"NoWritesPerformed",
"RetryableWriteError"
]
}
}
}
},
{
"name": "insertOne",
"object": "collection0",
"arguments": {
"document": {
"x": 1
}
},
"expectError": {
"errorCode": 64,
"errorLabelsContain": [
"NoWritesPerformed",
"RetryableWriteError"
]
}
}
],
"outcome": [
{
"collectionName": "no-writes-performed-collection",
"databaseName": "retryable-writes-tests",
"documents": []
}
]
}
]
}
59 changes: 59 additions & 0 deletions test/test_retryable_writes.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from test.utils import (
CMAPListener,
DeprecationFilter,
EventListener,
OvertCommandListener,
TestCreator,
rs_or_single_client,
Expand All @@ -45,6 +46,7 @@
)
from pymongo.mongo_client import MongoClient
from pymongo.monitoring import (
CommandSucceededEvent,
ConnectionCheckedOutEvent,
ConnectionCheckOutFailedEvent,
ConnectionCheckOutFailedReason,
Expand All @@ -64,6 +66,26 @@
_TEST_PATH = os.path.join(os.path.dirname(os.path.realpath(__file__)), "retryable_writes", "legacy")


class InsertEventListener(EventListener):
def succeeded(self, event: CommandSucceededEvent) -> None:
super(InsertEventListener, self).succeeded(event)
if (
event.command_name == "insert"
and event.reply.get("writeConcernError", {}).get("code", None) == 91
):
client_context.client.admin.command(
{
"configureFailPoint": "failCommand",
"mode": {"times": 1},
"data": {
"errorCode": 10107,
"errorLabels": ["RetryableWriteError", "NoWritesPerformed"],
"failCommands": ["insert"],
},
}
)


class TestAllScenarios(SpecRunner):
RUN_ON_LOAD_BALANCER = True
RUN_ON_SERVERLESS = True
Expand Down Expand Up @@ -581,6 +603,43 @@ 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_context.require_version_min(
6, 0, 0
) # the spec requires that this prose test only be run on 6.0+
@client_knobs(heartbeat_frequency=0.05, min_heartbeat_interval=0.05)
def test_returns_original_error_code(
self,
):
cmd_listener = InsertEventListener()
client = rs_or_single_client(retryWrites=True, event_listeners=[cmd_listener])
client.test.test.drop()
self.addCleanup(client.close)
cmd_listener.reset()
client.admin.command(
{
"configureFailPoint": "failCommand",
"mode": {"times": 1},
"data": {
"writeConcernError": {
"code": 91,
"errorLabels": ["RetryableWriteError"],
},
"failCommands": ["insert"],
},
}
)
with self.assertRaises(WriteConcernError) as exc:
client.test.test.insert_one({"_id": 1})
self.assertEqual(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):
Expand Down