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

Expose API for providing a custom Connect impl and pass through the runtime feature #316

Merged
merged 3 commits into from May 4, 2024

Conversation

shadaj
Copy link
Contributor

@shadaj shadaj commented Apr 15, 2024

Currently, deadpool-postgres only offers APIs for creating pools that instantiate their underlying connections based on a tokio_postgres::Config. In some situations (mocking, custom networking), it is useful to provide an alternate connection mechanism. Thankfully, deadpool-postgres already has the Client trait for this so we just need to expose it publicly. As part of this, we also expose the existing ConnectImpl as a public ConfigConnectImpl for code that needs to abstract over a selection of connection strategy.

Our main use case for this is to enable use on WASM, where tokio_postgres is already supported but does not have the tokio-postgres/runtime feature enabled (which enables the APIs for creation based on a database URL). So this PR also adds a runtime feature that maps to the underlying tokio-postgres one for now (defaulting to true to preserve compatibility). It also includes a minor patch to disable the keepalives_idle config on WASM where it is not supported.

Copy link
Owner

@bikeshedder bikeshedder left a comment

Choose a reason for hiding this comment

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

LGTM. I'm just curious if we really need to feature gate so much of the crate. I would have thought that the feature gated functions would not cause any issues for the WASM platform as they are generic over T.

How can we add tests for this? It would be really nice to make sure future additions to this crate don't break WASM support.

@@ -14,7 +14,8 @@ readme = "README.md"
all-features = true

[features]
default = ["rt_tokio_1"]
default = ["runtime", "rt_tokio_1"]
runtime = ["tokio-postgres/runtime"]
Copy link
Owner

Choose a reason for hiding this comment

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

How about making this a dependency of the rt_tokio_1 and rt_async-std_1 feature instead? That way the default feature set will just stay the same and it won't break for users of rs_async-std_1 either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea! Will update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or actually, on second thought, this won't work unfortunately. Because for example on WASM, we do want rt_tokio_1 if we're running on Tokio, just not the runtime feature.

@@ -82,6 +89,7 @@ pub struct Manager {
}

impl Manager {
#[cfg(feature = "runtime")]
Copy link
Owner

Choose a reason for hiding this comment

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

Do you really need to feature gate all of this? As the feature gated functions are generic over T it shouldn't cause any harm, does it? Is the core of the issue the tokio::spawn which isn't supported by the WASM target?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this one (and its friends) need to be gated because they all initialize the Postgres connection through the tokio_postgres::Config. That's what's not supported when you don't have the runtime feature on tokio_postgres (like on WASM), since it requires tokio_postgres to establish the TCP connection on your behalf. So in a world without runtime, we can only keep around the APIs where you bring your own connection.

@shadaj
Copy link
Contributor Author

shadaj commented Apr 20, 2024

For tests, let me look into adding CI logic to test at least compilation on WASM.

@shadaj
Copy link
Contributor Author

shadaj commented Apr 20, 2024

In theory, the new CI config should test compilation on WASM!

@bikeshedder bikeshedder merged commit 776b59f into bikeshedder:master May 4, 2024
62 of 65 checks passed
@bikeshedder
Copy link
Owner

I just removed the runtime feature replacing it by conditional dependencies instead:

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
tokio-postgres = "0.7.9"

[target.'cfg(target_arch = "wasm32")'.dependencies]
tokio-postgres = { version = "0.7.9", default-features = false }

Could you please check if that works for you?

This should be a non breaking release for non-WASM targets now.

@shadaj
Copy link
Contributor Author

shadaj commented May 5, 2024

@bikeshedder oh actually I think this doesn't work, due to rust-lang/cargo#1197

In our downstream crates we are seeing the default features pulled in when compiling on WASM.

@bikeshedder bikeshedder mentioned this pull request May 7, 2024
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.

None yet

2 participants