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: all throwables should be ignored in shutdown hook #950

Merged
merged 1 commit into from Mar 16, 2021

Conversation

olavloite
Copy link
Contributor

@olavloite olavloite commented Mar 11, 2021

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
@olavloite olavloite requested a review from thiagotnunes Mar 11, 2021
@olavloite olavloite requested a review from as a code owner Mar 11, 2021
@google-cla google-cla bot added the cla: yes label Mar 11, 2021
@product-auto-label product-auto-label bot added the api: spanner label Mar 11, 2021
@codecov
Copy link

@codecov codecov bot commented Mar 11, 2021

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 Files Coverage Δ Complexity Δ
...m/google/cloud/spanner/connection/SpannerPool.java 87.36% <0.00%> (ø) 33.00 <0.00> (ø)
...cloud/spanner/connection/ReadWriteTransaction.java 82.11% <0.00%> (-0.28%) 59.00% <0.00%> (-1.00%)
...ud/spanner/SessionPoolAsyncTransactionManager.java 87.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
Copy link
Contributor

@thiagotnunes thiagotnunes commented Mar 15, 2021

Why is the class loader different during initialization vs finalization?

@olavloite
Copy link
Contributor Author

@olavloite olavloite commented Mar 15, 2021

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
Copy link
Contributor

@thiagotnunes thiagotnunes commented Mar 16, 2021

Thanks for the explanation @olavloite

@thiagotnunes thiagotnunes merged commit 213dddc into master Mar 16, 2021
18 of 19 checks passed
@thiagotnunes thiagotnunes deleted the ignore-throwables-in-shutdown-hook branch Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner cla: yes
Projects
None yet
2 participants