PYTHON-3388 Propagate Original Error for Write Errors Labeled NoWritesPerformed #1117
Conversation
pymongo/mongo_client.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
We can't use an instance variable to track this because it makes MongoClient not thread safe.
pymongo/mongo_client.py
Outdated
| if self._indefinite_error and exc.has_error_label("NoWritesPerformed"): | ||
| raise self._indefinite_error from exc | ||
| if isinstance(exc, WriteConcernError): | ||
| self._indefinite_error = exc |
There was a problem hiding this comment.
_indefinite_error is not needed. last_error already tracks the error that should be propagated.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
|
Mypy failure: pymongo/mongo_client.py:1416: error: Incompatible types in assignment
(expression has type "PyMongoError", variable has type
"Optional[WriteConcernError]") [assignment]
self._indefinite_error = exc |
pymongo/mongo_client.py
Outdated
| raise self._indefinite_error from exc | ||
| if isinstance(exc, WriteConcernError): | ||
| self._indefinite_error = exc | ||
| with self._indefinite_error_lock: |
There was a problem hiding this comment.
Adding a lock here does not help at all. It's still not thread safe. We can't use an instance variable to track something that should be a local variable.
| assert last_error is not None | ||
| raise last_error from exc | ||
| else: | ||
| raise |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Removing the assertion and refactoring fixed this.
I do not think this pre-commit failure is related. |
|
The pre-commit failure was addressed in #1122 |
No description provided.