Skip to content
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

fix: check for timeout in connection after last statement finished #1086

Merged
merged 1 commit into from Apr 25, 2021

Conversation

olavloite
Copy link
Collaborator

@olavloite olavloite commented Apr 22, 2021

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
@olavloite olavloite requested a review from a team as a code owner April 22, 2021 14:52
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Apr 22, 2021
@google-cla google-cla bot added the cla: yes This 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
Copy link

codecov bot commented Apr 22, 2021

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 Files Coverage Δ Complexity Δ
...cloud/spanner/connection/ReadWriteTransaction.java 81.81% <95.45%> (+0.46%) 72.00 <10.00> (+1.00)
.../google/cloud/spanner/AbstractLazyInitializer.java 92.85% <0.00%> (-7.15%) 4.00% <0.00%> (-1.00%)
...m/google/cloud/spanner/connection/SpannerPool.java 87.16% <0.00%> (-2.14%) 33.00% <0.00%> (ø%)
...ain/java/com/google/cloud/spanner/SessionPool.java 88.82% <0.00%> (ø) 73.00% <0.00%> (-1.00%)
...ud/spanner/SessionPoolAsyncTransactionManager.java 86.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.

@thiagotnunes thiagotnunes merged commit aec0b54 into master Apr 25, 2021
@thiagotnunes thiagotnunes 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spanner.connection.StatementTimeoutTest: testTimeoutExceptionReadWriteTransactionMultipleStatements failed
2 participants