Conversation

olavloite

The check whether the previous statement timed out in the Connection API was
done when a statement was submitted to the connection, and not when the statement
was executed. That could cause a race condition, as statements are executed using
a separate thread, while submitting a statement is done using the main thread.

This could cause a statement to return an error with code ABORTED instead of
FAILED_PRECONDITION. A statement on a read/write transaction would always return
an error when a/the previous statement timed out, only the error code could
be different depending on whether the race condition occurred or not. This is
now fixed so that the error code is always FAILED_PRECONDITION and the error
indicates that a read/write transaction is no longer usable after a statement
timeout.

Fixes #1077 and #738

The check whether the previous statement timed out in the Connection API was
done when a statement was submitted to the connection, and not when the statement
was executed. That could cause a race condition, as statements are executed using
a separate thread, while submitting a statement is done using the main thread.

This could cause a statement to return an error with code ABORTED instead of
FAILED_PRECONDITION. A statement on a read/write transaction would always return
an error when a/the previous statement timed out, only the error code could
be different depending on whether the race condition occurred or not. This is
now fixed so that the error code is always FAILED_PRECONDITION and the error
indicates that a read/write transaction is no longer usable after a statement
timeout.

Fixes #1077
@olavloiteolavloite requested a review from a team as a code owner April 22, 2021 14:52
@product-auto-labelproduct-auto-label bot added the api: spannerIssues related to the googleapis/java-spanner API.label Apr 22, 2021
@google-clagoogle-cla bot added the cla: yesThis human has signed the Contributor License Agreement.label Apr 22, 2021
}
}),
() -> {
checkTimedOut();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These changes seem bigger than they are. The only real difference is that the checkTimeout() call is added as the first statement of the lambda.

@codecov

Codecov Report

Merging #1086 (f0bb299) into master (87e68c7) will decrease coverage by 0.01%.
The diff coverage is 95.45%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1086      +/-   ##
============================================
- Coverage     85.15%   85.13%   -0.02%     
- Complexity     2716     2718       +2     
============================================
  Files           155      155              
  Lines         14351    14360       +9     
  Branches       1358     1357       -1     
============================================
+ Hits          12220    12226       +6     
- Misses         1563     1566       +3     
  Partials        568      568              
Impacted FilesCoverage ΔComplexity Δ
...cloud/spanner/connection/ReadWriteTransaction.java81.81% <95.45%> (+0.46%)72.00 <10.00> (+1.00)
.../google/cloud/spanner/AbstractLazyInitializer.java92.85% <0.00%> (-7.15%)4.00% <0.00%> (-1.00%)
...m/google/cloud/spanner/connection/SpannerPool.java87.16% <0.00%> (-2.14%)33.00% <0.00%> (ø%)
...ain/java/com/google/cloud/spanner/SessionPool.java88.82% <0.00%> (ø)73.00% <0.00%> (-1.00%)
...ud/spanner/SessionPoolAsyncTransactionManager.java86.61% <0.00%> (+1.57%)15.00% <0.00%> (+2.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87e68c7...f0bb299. Read the comment docs.

@thiagotnunesthiagotnunes merged commit aec0b54 into master Apr 25, 2021
@thiagotnunesthiagotnunes deleted the check-for-timeout-in-callable branch April 25, 2021 02:48
renovate-bot pushed a commit to renovate-bot/java-spanner that referenced this pull request Apr 25, 2021
…oogleapis#1086)

The check whether the previous statement timed out in the Connection API was
done when a statement was submitted to the connection, and not when the statement
was executed. That could cause a race condition, as statements are executed using
a separate thread, while submitting a statement is done using the main thread.

This could cause a statement to return an error with code ABORTED instead of
FAILED_PRECONDITION. A statement on a read/write transaction would always return
an error when a/the previous statement timed out, only the error code could
be different depending on whether the race condition occurred or not. This is
now fixed so that the error code is always FAILED_PRECONDITION and the error
indicates that a read/write transaction is no longer usable after a statement
timeout.

Fixes googleapis#1077
@olavloiteolavloite mentioned this pull request May 3, 2021
Sign up for free to join this conversation on . Already have an account? Sign in to comment
api: spannerIssues related to the googleapis/java-spanner API.cla: yesThis human has signed the Contributor License Agreement.
None yet

Successfully merging this pull request may close these issues.

spanner.connection.StatementTimeoutTest: testTimeoutExceptionReadWriteTransactionMultipleStatements failed