Conversation

olavloite

All throwables (and not just exceptions) should be ignored in the shutdown hook. Failing to close these resources during shutdown is not a major problem, as they will be garbage collected by the backend anyways. Without this wide catch, some
applications will log a ClassNotFoundException when shutting down, which can be confusing for end users.

Fixes #949

Also fixes cloudspannerecosystem/liquibase-spanner#66 (comment)

All throwables (and not just exceptions) should be ignored in the shutdown hook.
Failing to close these resources during shutdown is not a major problem, as they
will be garbage collected by the backend anyways. Without this wide catch, some
applications will log a ClassNotFoundException when shutting down, which can be
confusing for end users.

Fixes #949
@olavloiteolavloite requested a review from a team as a code owner March 11, 2021 10:57
@google-clagoogle-cla bot added the cla: yesThis human has signed the Contributor License Agreement.label Mar 11, 2021
@product-auto-labelproduct-auto-label bot added the api: spannerIssues related to the googleapis/java-spanner API.label Mar 11, 2021
@codecov

Codecov Report

Merging #950 (06e24a8) into master (4088981) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #950   +/-   ##
=========================================
  Coverage     85.22%   85.22%           
- Complexity     2651     2652    +1     
=========================================
  Files           145      145           
  Lines         14358    14358           
  Branches       1391     1391           
=========================================
+ Hits          12236    12237    +1     
+ Misses         1540     1539    -1     
  Partials        582      582           
Impacted FilesCoverage ΔComplexity Δ
...m/google/cloud/spanner/connection/SpannerPool.java87.36% <0.00%> (ø)33.00 <0.00> (ø)
...cloud/spanner/connection/ReadWriteTransaction.java82.11% <0.00%> (-0.28%)59.00% <0.00%> (-1.00%)
...ud/spanner/SessionPoolAsyncTransactionManager.java87.21% <0.00%> (+1.50%)14.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 4088981...06e24a8. Read the comment docs.

@thiagotnunes

Why is the class loader different during initialization vs finalization?

@olavloite

Why is the class loader different during initialization vs finalization?

I'm not sure exactly what happens, but globally speaking it seems to be caused by the fact that the shutdown hook is invoked when Maven is being shutdown, and not when the plugin is shutdown. So my hunch is the following:

  1. Maven starts and runs the Liquibase plugin.
  2. The Liquibase plugin dynamically loads the JDBC drivers and other services that it can find and then does its work.
  3. The Liquibase plugin shuts down, but that does not shutdown the JVM.
  4. Maven shuts down, which also shuts down the JVM, and that invokes the shutdown hook of the JDBC driver. But Maven does not know that the JDBC driver had been dynamically loaded by Liquibase.

Note: This is not a finalizer of a class, but a JVM shutdown hook, so it is only invoked when the entire JVM is shut down.

@thiagotnunes

Thanks for the explanation @olavloite

@thiagotnunesthiagotnunes merged commit 213dddc into master Mar 16, 2021
@thiagotnunesthiagotnunes deleted the ignore-throwables-in-shutdown-hook branch March 16, 2021 23:51
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.

SpannerPool shutdown hook can fail with ClassNotFoundException java.lang.NoClassDefFoundError when running mvn liquibase:update