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

[CT-1233] [Bug] Race condition in lib.py causes concurrent API calls to fail #5919

Closed
2 tasks done
drewbanin opened this issue Sep 23, 2022 · 1 comment · Fixed by #5921
Closed
2 tasks done

[CT-1233] [Bug] Race condition in lib.py causes concurrent API calls to fail #5919

drewbanin opened this issue Sep 23, 2022 · 1 comment · Fixed by #5921
Labels
bug Something isn't working

Comments

@drewbanin
Copy link
Contributor

Is this a new bug in dbt-core?

  • I believe this is a new bug in dbt-core
  • I have searched the existing issues, and I could not find an existing issue for this bug

Current Behavior

The lib.py file in Core contains logic to "reset adapters" in the process of generating a RuntimeConfig object. While this logic is appropriate for single-threaded usage of dbt Core, we've seen that concurrent api calls to this method can result in race conditions:

dbt-core/core/dbt/lib.py

Lines 27 to 32 in 5678344

# Clear previously registered adapters--
# this fixes cacheing behavior on the dbt-server
flags.set_from_args(args, config)
dbt.adapters.factory.reset_adapters()
# Load the relevant adapter
dbt.adapters.factory.register_adapter(config)

The reset_adapters call will clear out the set of adapters registered in the dbt Core adapter factory. If a concurrent api call accesses adapters (eg. to compile sql, or run dbt, or similar) before the register_adapters call is complete, there's a chance it will fail with a KeyError eg. here because the adapter dictionary was cleared in another thread.

Expected Behavior

The get_dbt_config should not mutate global state to generate a RuntimeConfig.

Steps To Reproduce

Here's a minimal repro case:

import dbt.lib
import threading
import time
import random
import dbt.adapters.factory

PROJECT_PATH = "/Users/drew/dev/internal-analytics"


def repro_func(thread_no):
    time.sleep(random.random())

    try:
        # Simulate concurrent use of adapters
        config = dbt.lib.get_dbt_config(PROJECT_PATH)
        adapter = dbt.adapters.factory.get_adapter_by_type(config.credentials.type)
        adapter.acquire_connection()
        adapter.execute('select 1 as id')

        print(f"Thread {thread_no} OK")
    except KeyError:
        print("ERROR: KeyError")


threads = []
for i in range(25):
    thread = threading.Thread(target=repro_func, args=(i,))
    thread.start()
    threads.append(thread)

for thread in threads:
    thread.join()

Screen Shot 2022-09-23 at 9 51 27 AM

Relevant log output

No response

Environment

- OS: macOS
- Python: 3.9.14
- dbt: 1.2.1

Which database adapter are you using with dbt?

postgres, redshift, snowflake, bigquery, spark

Additional Context

I am interested in submitting a patch for this issue

@drewbanin drewbanin added bug Something isn't working triage labels Sep 23, 2022
@github-actions github-actions bot changed the title [Bug] Race condition in lib.py causes concurrent API calls to fail [CT-1233] [Bug] Race condition in lib.py causes concurrent API calls to fail Sep 23, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Sep 23, 2022

Thanks for opening @drewbanin!

I think you have a short-term patch for this, which feels appropriate for now.

@iknox-fa and I have been chatting about this a bit, most recently over in #5533. De-globalizing adapters, while not in the immediate scope of API-ification, is something I'd like to tackle in the next few months. I think we can disambiguate between:

  • "plugin": a driver, appropriate to register globally at application startup, and then treat as library code for use in specific invocations
  • "adapter": an object initialized using plugin (library) code, using config/credentials that might be specific to one invocation — and should not be treated as a singleton registered/cleared globally

The get_dbt_config should not mutate global state to generate a RuntimeConfig.

Aside from registering/clearing adapters, there are two other pieces of global state mutation in get_dbt_config:

  • Resetting the flags module (global)
  • Resetting the invocation_id (global in the events module)

Those are both things we'll want to "de-globalize," in addition to adapters, and I sense they would be smaller lifts.

@jtcohen6 jtcohen6 removed the triage label Sep 23, 2022
drewbanin added a commit that referenced this issue Sep 26, 2022
* (#5919) Fix adapter reset race condition in lib.py

* run black

* changie
github-actions bot pushed a commit that referenced this issue Sep 26, 2022
* (#5919) Fix adapter reset race condition in lib.py

* run black

* changie

(cherry picked from commit 4e8aa00)
github-actions bot pushed a commit that referenced this issue Sep 26, 2022
* (#5919) Fix adapter reset race condition in lib.py

* run black

* changie

(cherry picked from commit 4e8aa00)
leahwicz pushed a commit that referenced this issue Sep 27, 2022
* (#5919) Fix adapter reset race condition in lib.py

* run black

* changie

(cherry picked from commit 4e8aa00)

Co-authored-by: Drew Banin <drew@dbtlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants