Skip to content

Commit aa19fc6

Browse files
wilhuffwilhuff
authored
Treat ABORTED writes as retryable (#189)
This corresponds to a server-side change that will be released for the v1 protocol only (see b/119437764). This is a port of firebase/firebase-js-sdk#1456.
1 parent d26f7e1 commit aa19fc6

File tree

4 files changed

+348
-56
lines changed

4 files changed

+348
-56
lines changed

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/Datastore.java

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,13 @@ public Task<List<MaybeDocument>> lookup(List<DocumentKey> keys) {
219219
});
220220
}
221221

222-
public static boolean isPermanentWriteError(Status status) {
222+
/**
223+
* Determines whether the given status has an error code that represents a permanent error when
224+
* received in response to a non-write operation.
225+
*
226+
* @see #isPermanentWriteError for classifying write errors.
227+
*/
228+
public static boolean isPermanentError(Status status) {
223229
// See go/firestore-client-errors
224230
switch (status.getCode()) {
225231
case OK:
@@ -251,4 +257,19 @@ public static boolean isPermanentWriteError(Status status) {
251257
throw new IllegalArgumentException("Unknown gRPC status code: " + status.getCode());
252258
}
253259
}
260+
261+
/**
262+
* Determines whether the given status has an error code that represents a permanent error when
263+
* received in response to a write operation.
264+
*
265+
* <p>Write operations must be handled specially because as of b/119437764, ABORTED errors on the
266+
* write stream should be retried too (even though ABORTED errors are not generally retryable).
267+
*
268+
* <p>Note that during the initial handshake on the write stream an ABORTED error signals that we
269+
* should discard our stream token (i.e. it is permanent). This means a handshake error should be
270+
* classified with isPermanentError, above.
271+
*/
272+
public static boolean isPermanentWriteError(Status status) {
273+
return isPermanentError(status) && !status.getCode().equals(Status.Code.ABORTED);
274+
}
254275
}

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteStore.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
import com.google.firebase.firestore.util.Util;
3838
import com.google.protobuf.ByteString;
3939
import io.grpc.Status;
40-
import io.grpc.Status.Code;
4140
import java.util.ArrayDeque;
4241
import java.util.Deque;
4342
import java.util.HashMap;
@@ -642,9 +641,10 @@ private void handleWriteStreamClose(Status status) {
642641

643642
private void handleWriteHandshakeError(Status status) {
644643
hardAssert(!status.isOk(), "Handling write error with status OK.");
645-
// Reset the token if it's a permanent error or the error code is ABORTED, signaling the write
646-
// stream is no longer valid.
647-
if (Datastore.isPermanentWriteError(status) || status.getCode().equals(Code.ABORTED)) {
644+
// Reset the token if it's a permanent error, signaling the write stream is no longer valid.
645+
// Note that the handshake does not count as a write: see comments on isPermanentWriteError for
646+
// details.
647+
if (Datastore.isPermanentError(status)) {
648648
String token = Util.toDebugString(writeStream.getLastStreamToken());
649649
Logger.debug(
650650
LOG_TAG,
@@ -658,7 +658,7 @@ private void handleWriteHandshakeError(Status status) {
658658

659659
private void handleWriteError(Status status) {
660660
hardAssert(!status.isOk(), "Handling write error with status OK.");
661-
// Only handle permanent error, if it's transient just let the retry logic kick in.
661+
// Only handle permanent errors here. If it's transient, just let the retry logic kick in.
662662
if (Datastore.isPermanentWriteError(status)) {
663663
// If this was a permanent error, the request itself was the problem so it's not going
664664
// to succeed if we resend it.

‎firebase-firestore/src/test/resources/json/listen_spec_test.json

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10053,5 +10053,211 @@
1005310053
"clientIndex": 1
1005410054
}
1005510055
]
10056+
},
10057+
"Previous primary immediately regains primary lease": {
10058+
"describeName": "Listens:",
10059+
"itName": "Previous primary immediately regains primary lease",
10060+
"tags": [
10061+
"multi-client"
10062+
],
10063+
"config": {
10064+
"useGarbageCollection": false,
10065+
"numClients": 2
10066+
},
10067+
"steps": [
10068+
{
10069+
"drainQueue": true,
10070+
"clientIndex": 0
10071+
},
10072+
{
10073+
"userListen": [
10074+
2,
10075+
{
10076+
"path": "collection",
10077+
"filters": [],
10078+
"orderBys": []
10079+
}
10080+
],
10081+
"stateExpect": {
10082+
"activeTargets": {
10083+
"2": {
10084+
"query": {
10085+
"path": "collection",
10086+
"filters": [],
10087+
"orderBys": []
10088+
},
10089+
"resumeToken": ""
10090+
}
10091+
}
10092+
},
10093+
"clientIndex": 0
10094+
},
10095+
{
10096+
"watchAck": [
10097+
2
10098+
],
10099+
"clientIndex": 0
10100+
},
10101+
{
10102+
"watchEntity": {
10103+
"docs": [],
10104+
"targets": [
10105+
2
10106+
]
10107+
},
10108+
"clientIndex": 0
10109+
},
10110+
{
10111+
"watchCurrent": [
10112+
[
10113+
2
10114+
],
10115+
"resume-token-1000"
10116+
],
10117+
"clientIndex": 0
10118+
},
10119+
{
10120+
"watchSnapshot": {
10121+
"version": 1000,
10122+
"targetIds": []
10123+
},
10124+
"expect": [
10125+
{
10126+
"query": {
10127+
"path": "collection",
10128+
"filters": [],
10129+
"orderBys": []
10130+
},
10131+
"errorCode": 0,
10132+
"fromCache": false,
10133+
"hasPendingWrites": false
10134+
}
10135+
],
10136+
"clientIndex": 0
10137+
},
10138+
{
10139+
"drainQueue": true,
10140+
"clientIndex": 1
10141+
},
10142+
{
10143+
"applyClientState": {
10144+
"primary": true
10145+
},
10146+
"stateExpect": {
10147+
"isPrimary": true,
10148+
"activeTargets": {
10149+
"2": {
10150+
"query": {
10151+
"path": "collection",
10152+
"filters": [],
10153+
"orderBys": []
10154+
},
10155+
"resumeToken": "resume-token-1000"
10156+
}
10157+
}
10158+
},
10159+
"clientIndex": 1
10160+
},
10161+
{
10162+
"watchAck": [
10163+
2
10164+
],
10165+
"clientIndex": 1
10166+
},
10167+
{
10168+
"watchEntity": {
10169+
"docs": [
10170+
{
10171+
"key": "collection/a",
10172+
"version": 2000,
10173+
"value": {
10174+
"key": "a"
10175+
},
10176+
"options": {
10177+
"hasLocalMutations": false,
10178+
"hasCommittedMutations": false
10179+
}
10180+
}
10181+
],
10182+
"targets": [
10183+
2
10184+
]
10185+
},
10186+
"clientIndex": 1
10187+
},
10188+
{
10189+
"watchCurrent": [
10190+
[
10191+
2
10192+
],
10193+
"resume-token-2000"
10194+
],
10195+
"clientIndex": 1
10196+
},
10197+
{
10198+
"watchSnapshot": {
10199+
"version": 2000,
10200+
"targetIds": []
10201+
},
10202+
"clientIndex": 1
10203+
},
10204+
{
10205+
"shutdown": true,
10206+
"stateExpect": {
10207+
"activeTargets": {},
10208+
"limboDocs": []
10209+
},
10210+
"clientIndex": 1
10211+
},
10212+
{
10213+
"drainQueue": true,
10214+
"stateExpect": {
10215+
"isPrimary": true
10216+
},
10217+
"clientIndex": 0
10218+
},
10219+
{
10220+
"runTimer": "client_metadata_refresh",
10221+
"stateExpect": {
10222+
"isPrimary": true,
10223+
"activeTargets": {
10224+
"2": {
10225+
"query": {
10226+
"path": "collection",
10227+
"filters": [],
10228+
"orderBys": []
10229+
},
10230+
"resumeToken": "resume-token-2000"
10231+
}
10232+
}
10233+
},
10234+
"expect": [
10235+
{
10236+
"query": {
10237+
"path": "collection",
10238+
"filters": [],
10239+
"orderBys": []
10240+
},
10241+
"added": [
10242+
{
10243+
"key": "collection/a",
10244+
"version": 2000,
10245+
"value": {
10246+
"key": "a"
10247+
},
10248+
"options": {
10249+
"hasLocalMutations": false,
10250+
"hasCommittedMutations": false
10251+
}
10252+
}
10253+
],
10254+
"errorCode": 0,
10255+
"fromCache": false,
10256+
"hasPendingWrites": false
10257+
}
10258+
],
10259+
"clientIndex": 0
10260+
}
10261+
]
1005610262
}
1005710263
}

0 commit comments

Comments
 (0)