bb8 icon indicating copy to clipboard operation
bb8 copied to clipboard

Ergonomics issue with PostgresConnectionManager

Open ufoscout opened this issue 5 years ago • 4 comments

Hi,

this issue is conceptually linked to https://github.com/sfackler/r2d2-postgres/issues/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 https://github.com/sfackler/r2d2-postgres/issues/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?

ufoscout avatar Jan 24 '20 09:01 ufoscout

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?

khuey avatar Jan 24 '20 14:01 khuey

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?

t3hmrman avatar Apr 14 '20 08:04 t3hmrman

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?

leafypout avatar Jan 18 '23 13:01 leafypout

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.

djc avatar Jan 18 '23 13:01 djc