Skip to content

Ensure tests don't leak database connections #1216

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

Merged
merged 2 commits into from
Dec 29, 2020

Conversation

Nemo157
Copy link
Member

@Nemo157 Nemo157 commented Dec 23, 2020

Even if a pool instance is leaked (such as when the web server is leaked), internally drop the actual pool at the end of the test to ensure all connections are closed.

Even if a pool instance is leaked (such as when the web server is
leaked), internally drop the actual pool at the end of the test to
ensure all connections are closed.
@jyn514 jyn514 mentioned this pull request Dec 25, 2020
@jyn514 jyn514 added A-newcomer-roadblock Area: A problem that isn't a bug, but makes it harder for people to contribute S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Dec 25, 2020
@jyn514
Copy link
Member

jyn514 commented Dec 25, 2020

I still can't run all tests because we leak file descriptors, but I guess there's not much to do about that ... ideally we'd switch to warp or hyper so this wasn't an issue :/

@Nemo157
Copy link
Member Author

Nemo157 commented Dec 25, 2020

I might have fixed that when reducing the number of threads that the tests take, I could pull that commit into this PR too if you want?

@jyn514
Copy link
Member

jyn514 commented Dec 25, 2020

I might have fixed that when reducing the number of threads that the tests take, I could pull that commit into this PR too if you want?

That would be great!

It defaults to 8 * num_cpus, and we leak these threads since we can't
shut iron down, so on my 20-core machine it was hitting 11,500 idle
threads during the test run.
@jyn514
Copy link
Member

jyn514 commented Dec 25, 2020

This still leaks file descriptors for me :/ I guess if it fixes CI it's worth it, though.

thread 'r2d2-worker-1' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 24, kind: Other, message: "Too many open files" }', /home/joshua/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/postgres-0.17.5/src/config.rs:322:14

@jyn514
Copy link
Member

jyn514 commented Dec 25, 2020

Oh wait, I forgot to set ulimit -n 4096 first. Now it works :)

@jyn514
Copy link
Member

jyn514 commented Dec 25, 2020

r? @pietroalbini

This seems ok to me, but I'm still a little hesitant about adding hacks on top of hacks when @Nemo157's been working on the warp switch: master...Nemo157:warp-speed-iron

@Nemo157
Copy link
Member Author

Nemo157 commented Dec 25, 2020

Yeah, it doesn't stop leakage, but it does reduce it by a multiplier of something like 4*num_cpus.

The warp switch is going to take a long time to finish, and all these hacks can be deleted once it's completed.

@pietroalbini
Copy link
Member

I hate this, let's merge it 👍

@jyn514 jyn514 merged commit 15860cc into rust-lang:master Dec 29, 2020
@Nemo157 Nemo157 deleted the leak-free-pool branch December 29, 2020 08:05
@jyn514 jyn514 added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it labels Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-newcomer-roadblock Area: A problem that isn't a bug, but makes it harder for people to contribute
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants