xds: update nonce even if the ACK/NACK is not sent on wire #3497
Conversation
…K is not sent on wire This can happen when the watch is canceled while the response is on wire.
| callbackCh.Send(struct{}{}) | ||
| }) | ||
| if err := compareXDSRequest(fakeServer.XDSRequestChan, goodCDSRequest, "", ""); err != nil { | ||
| t.Fatalf("Failed to receive %s request: %v", "CDS", err) |
There was a problem hiding this comment.
Nit: Could this just be:
t.Fatalf("Failed to receive CSD request: %v", err)
There was a problem hiding this comment.
Or it could just be t.Fatal(err) since the compareXDSRequest() function properly annotates the error that it returns.
|
|
||
| // Send a good CDS response, this function waits for the ACK with the right | ||
| // version. | ||
| nonce := sendGoodResp(t, "CDS", fakeServer, versionCDS, goodCDSResponse1, goodCDSRequest, callbackCh) |
There was a problem hiding this comment.
The helper functions in this test file seem to be a little inconsistent in the sense that some just return errors while others call t.Error/Fatal. Given that none of these functions are helpers in the true sense i.e functions whose actions do not depend on the current test, I think we should make them all just return error and return error which are complete with the information as to why the error happened, and thereby the actual test can simply call t.Fatal(err).
There was a problem hiding this comment.
I can either make them all return error, or make them all call t.Fatal, and mark them as helper().
I added a TODO, and will leave it for later.
|
|
||
| // TestV2ClientAckCancelResponseRace verifies if the response and ACK request | ||
| // race with cancel (which means the ACK request will not be sent on wire, | ||
| // because there's no active watch), the version will still be updates, and the |
There was a problem hiding this comment.
Done.
The comment is actually not accurate. Only the nonce will be updated. The version will not if cancel happens before ACK.
| callbackCh.Send(struct{}{}) | ||
| }) | ||
| if err := compareXDSRequest(fakeServer.XDSRequestChan, goodCDSRequest, "", ""); err != nil { | ||
| t.Fatalf("Failed to receive %s request: %v", "CDS", err) |
|
|
||
| // Send a good CDS response, this function waits for the ACK with the right | ||
| // version. | ||
| nonce := sendGoodResp(t, "CDS", fakeServer, versionCDS, goodCDSResponse1, goodCDSRequest, callbackCh) |
There was a problem hiding this comment.
I can either make them all return error, or make them all call t.Fatal, and mark them as helper().
I added a TODO, and will leave it for later.
|
|
||
| // TestV2ClientAckCancelResponseRace verifies if the response and ACK request | ||
| // race with cancel (which means the ACK request will not be sent on wire, | ||
| // because there's no active watch), the version will still be updates, and the |
There was a problem hiding this comment.
Done.
The comment is actually not accurate. Only the nonce will be updated. The version will not if cancel happens before ACK.
This can happen when the watch is canceled while the response is on wire. Also, tag ACK/NACK with the stream so nonce for a new stream doesn't get updated by a ACK from the previous stream.
This can happen when the watch is canceled while the response is on wire.
Also, tag ACK/NACK with the stream so nonce for a new stream doesn't get updated by a ACK from the previous stream.