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 race condition in pool close (#3217) #3299

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

madadam
Copy link
Contributor

@madadam madadam commented Jun 19, 2024

Attempt to fix #3217.

@abonander
Copy link
Collaborator

@madadam if you rebase it should fix the CI failure.

@madadam madadam force-pushed the pool-close-race-condition branch from eaaa536 to cdb5707 Compare August 8, 2024 12:05
@abonander
Copy link
Collaborator

Given that the PgListener test is consistently failing even after multiple re-runs, I'm wondering if there's some subtle problem with the fix here.

@madadam madadam force-pushed the pool-close-race-condition branch from cdb5707 to 5a6d8b5 Compare February 6, 2025 12:07
@madadam
Copy link
Contributor Author

madadam commented Feb 6, 2025

Finally found some time to look into this. The test was failing due to a deadlock: There was still one checked out connection inside the PgListener and so Pool::close was waiting for it to be released which never happened. The reason this was passing before is that the test accidentally relied on the old buggy behaviour of Pool::close where it didn't always wait for all connections to close. I fixed the test, rebased against main and updated the PR.

@abonander
Copy link
Collaborator

That's weird, now some of the migrations tests are timing out.

@madadam
Copy link
Contributor Author

madadam commented Feb 10, 2025

Yeah I noticed. I'll try to look into it when I can. Btw, how do you guys run these tests locally? I noticed that tests/x.py doesn't run the same test suite as what's run on the CI. In fact, I'm getting a compile error currently:

# unit test core
 $ cargo test --no-default-features --manifest-path sqlx-core/Cargo.toml --features json,offline,migrate,_rt-async-std,_tls-rustls 
warning: /home/adam/projects/sqlx/Cargo.toml: file `/home/adam/projects/sqlx/tests/sqlite/macros.rs` found to be present in multiple build targets:
  * `integration-test` target `sqlite-macros`
  * `integration-test` target `sqlite-unbundled-macros`
warning: /home/adam/projects/sqlx/sqlx-macros-core/Cargo.toml: unused manifest key: lints.rust.unexpected_cfgs.check-cfg
   Compiling sqlx-core v0.8.3 (/home/adam/projects/sqlx/sqlx-core)
error[E0425]: cannot find value `provider` in this scope
   --> sqlx-core/src/net/tls/tls_rustls.rs:107:54
    |
107 |     let config = ClientConfig::builder_with_provider(provider.clone())
    |                                                      ^^^^^^^^ not found in this scope

Also, trying to run a single target using the --target option throws exception:

# test postgres 17
Traceback (most recent call last):
  File "/home/adam/projects/sqlx/tests/./x.py", line 179, in <module>
    run(
  File "/home/adam/projects/sqlx/tests/./x.py", line 90, in run
    database_url = start_database(service, database="sqlite/sqlite.db" if service == "sqlite" else "sqlx", cwd=dir_tests)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/adam/projects/sqlx/tests/docker.py", line 24, in start_database
    res = subprocess.run(
          ^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/subprocess.py", line 548, in run
    with Popen(*popenargs, **kwargs) as process:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/subprocess.py", line 1026, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.12/subprocess.py", line 1955, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'docker-compose'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pool::close does not always wait for all connections to close
2 participants