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

Ergonomics issue with PostgresConnectionManager #57

Open
ufoscout opened this issue Jan 24, 2020 · 4 comments
Open

Ergonomics issue with PostgresConnectionManager #57

ufoscout opened this issue Jan 24, 2020 · 4 comments

Comments

@ufoscout
Copy link

Hi,

this issue is conceptually linked to sfackler/r2d2-postgres#19

The problem is related to the fact that the PostgresConnectionManager<T> struct has a generic parameter <T>.
If <T> is known at compile-time, then there are no problems; on the contrary, if it is only known at runtime then it has to be redeclared and propagated in every place where the connection and/or the Pool is used.

As already shown in sfackler/r2d2-postgres#19 , it is possible to slightly alter the PostgresConnectionManager struct implementation to remove the generic T and making the pool more ergonomic to use.

If you think it is not feasible, would you at least mind to provide both the alternatives of the PostgresConnectionManager, one with and one without the generic param?

@khuey
Copy link
Collaborator

khuey commented Jan 24, 2020

I wonder if this is really a bug with tokio-postgres. If it had an impl MakeTlsConnect for Box<dyn MakeTlsConnect>, then you could just box whatever you have into a trait object and things would work.

Any thoughts @sfackler?

@t3hmrman
Copy link

t3hmrman commented Apr 14, 2020

Just want to pile on here to say I'm having the same issue -- in fact I'm trying to store the connection manager and the pool that gets created. At first I tried to use the code that @ufoscout wrote in sfackler/r2d2-postgres#19, but I didn't want to stray that far from the beaten path so I ended up with this monstrosity:

type ConnMgr = PostgresConnectionManager<Box<dyn MakeTlsConnect<Socket, TlsConnect=dyn Send, Stream=dyn Send, Error=dyn Error>>>;

Now I'm just running into the same problem with Pool<M> where M is something that implements ManageConnection, and I'm struggling on how to fill the Connection and Error type variables. While I'm sure this really cleaned up the code for r2d2 it's made it a lot harder to use. Haven't been able to figure out how to do it for Pool so now I've moved making the pool to the area before I loop forever handling requests (it's a bit of an "actor"-ish setup).

[EDIT] - In the end I just stopped trying to own both of those in my own setup. This has made it really difficult to use -- implementing MakeTlsConnect for Box<dyn MakeTlsConnect> would definitely solve this it looks like though, since that's exactly what I"m missing -- is that OK as a path forward?

@levi-nz
Copy link

levi-nz commented Jan 18, 2023

I have nothing to add here, but I just wanted to comment that as a beginner to Rust, the usage of this is very confusing. Even as an experienced programmer just getting into Rust, it's extremely annoying to be able to accept any generic type for PostgresConnectionManager. From what I can tell, you can either use a type like NoTls directly (which I assume makes using TLS impossible?), or use the aforementioned type, which just looks ugly.

If you go for the second option above, you run into a huge problem when having your own custom error enums when implementing the From trait to turn say a RunError into your own error type, because you're forced into using generics.

Would it not be possible to just have some underlying connection manager of some sort that is capable of using TLS and non-TLS connections? I'm not sure how the internals work, but surely it would be possible to have some sort of abstraction to make this possible?

@djc
Copy link
Owner

djc commented Jan 18, 2023

It might be possible to write enums that wrap over well-known types implementing MakeTlsConnect, TlsConnect, AsyncRead, AsyncWrite and TlsStream. I probably won't be working on that, but if someone made fairly complete implementation I could review and merge it.

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

No branches or pull requests

5 participants