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

Added ThreadLocalScopeManager #3240

Merged
merged 4 commits into from
Sep 19, 2022
Merged

Conversation

nvoxland
Copy link
Contributor

@nvoxland nvoxland commented Sep 2, 2022

Impact

  • Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

By default, Liquibase uses SingletonScopeManager which is designed to be a single scope regardless of threads going on in the application. That works fine for single-threaded applications and use cases in a multi-threaded application, but for other multi-threaded applications it does not work.

This adds a liquibase.ThreadLocalScopeManager class which custom integrations can use to replace the default SingletonScopeManager with a call to Scope.setScopeManager(new ThreadLocalScopeManager()).

Things to be aware of

Things to worry about

  • Is there more areas of the code that is not thread safe?
    • The test is a relatively complete "end to end" test that hits a lot of the main Factories, but there can always be problems elsewhere
    • We can address them after this is released if/when anyone finds cases

Additional Context

Fixes #2018
Fixes #2248

@github-actions
Copy link

github-actions bot commented Sep 4, 2022

Unit Test Results

  4 632 files  +  12    4 632 suites  +12   40m 34s ⏱️ + 7m 32s
  4 723 tests +103    4 508 ✔️ +107     215 💤  - 4  0 ±0 
54 624 runs  +  12  49 604 ✔️ +  16  5 020 💤  - 4  0 ±0 

Results for commit f10b4e9. ± Comparison against base commit d87b435.

♻️ This comment has been updated with latest results.

@nvoxland
Copy link
Contributor Author

nvoxland commented Sep 5, 2022

The failing pro-tests and test-harness tests are unrelated to this change

Made SqlGeneratorFactory getGenerators() synchronized as it sometimes has concurrency issues
Made SqlGeneratorFactory getGenerators() synchronized as it sometimes has concurrency issues
@bvremmeinfor
Copy link

Hi @nvoxland, this looks good! Now there is hope :-)

Tested on https://github.com/bvremmeinfor/liquibase-threading-demo/tree/liquibase_pr_3240_test branch using snapshot build of #3240 (with public ThreadLocalScopeManager constructor).

Could simplify code in test project and use official implementations. Nice!

Repeated stress tests performed - 100% success.

Thank you!

Copy link
Contributor

@XDelphiGrl XDelphiGrl left a comment

Choose a reason for hiding this comment

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

This PR adds a new class, ThreadLocalScopeManager, to allow Liquibase integrations to leverage multiple threads. The default behavior for CLI and any integrations not opting to use the new ThreadLocalScopeManager class remains the same, i.e., uses a singleton created by SingletonScopeManager . The new test class FileSystemResourceAccessorTest is an excellent overview of how the new feature works.

APPROVED

  • Note: We had some build issues that prevented us from running all functional tests in a branch. The plan is to merge this to liquibase/liquibase master & re-run the tests. @nvoxland and I will be watching to see if there are adverse effects due to the merge. If so, we will fix or revert (depending on severity).

@nvoxland nvoxland merged commit 73332ad into master Sep 19, 2022
@nvoxland nvoxland deleted the added-threadlocal-scopemanager branch September 19, 2022 21:39
@kataggart kataggart added this to the NEXT milestone Sep 20, 2022
dvsp-iodigital added a commit to foreach-across/across-framework that referenced this pull request Dec 20, 2023
Package across-test.properties, which will be used by
TestDataSourceConfigurer, unless you have a
$HOME/dev-configs/across-test.properties file.

Upgrade liquibase and use ThreadLocalScopeManager to handle a
concurrency issue that occurs on the Amazon runners:
liquibase/liquibase#3240

Use GitLab CI/CD services for the databases instead of test
containers: this is an order of magnitude faster, especially for
Oracle, because it does not start and stop the entire database server
for each new spring test context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

ScopeManager can get confused when dealing with a heavy thread load. Create ThreadLocal ScopeManager
5 participants