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

Simplify server pool #177

Merged
merged 3 commits into from
Sep 15, 2023
Merged

Simplify server pool #177

merged 3 commits into from
Sep 15, 2023

Conversation

kornelski
Copy link
Contributor

Improvement for #171

@lipanski
Copy link
Owner

@kornelski what would be the minimum supported Rust version for this?

pub(crate) static ref SERVER_POOL: ServerPool = ServerPool::new(DEFAULT_POOL_SIZE);
}
// macOS has small default ulimits
const DEFAULT_POOL_SIZE: usize = if cfg!(target_os = "macos") { 20 } else { 50 };
Copy link
Owner

Choose a reason for hiding this comment

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

there's test_server_pool_async and test_server_pool that might fail on a Mac now => please update the tests to use the values conditionally as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests happen to pass currently, because they have a bug — the servers vec is emptied on every iteration, so they never exhaust the pool.

But when these tests actually try to get the whole pool, they cause a deadlock whenever any other test runs in parallel that uses more than one server at a time.

Copy link
Owner

Choose a reason for hiding this comment

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

right, that vec was supposed to be outside the for loop 🤦

@kornelski
Copy link
Contributor Author

Removal of lazy_static makes it 1.68.0 (vecdeque::new)

@lipanski
Copy link
Owner

Removal of lazy_static makes it 1.68.0 (vecdeque::new)

That should be fine. Please update the new min supported toolchain in the CI files & the README

@lipanski lipanski merged commit be00de4 into lipanski:master Sep 15, 2023
@lipanski
Copy link
Owner

Nice improvement, thank you 👍

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.

2 participants