prisma / quaint

SQL Query AST and Visitor for Rust
Apache License 2.0
583 stars 61 forks source link

Bad connection reused between health checks #273

Closed Sytten closed 3 years ago

Sytten commented 3 years ago

Consider the following path:

  1. Client ask Quaint for a connection
  2. Quaint ask mobc for a connectio
  3. If connection was checked recently (15s timeout in engine), return it directly
  4. Client tries to use connection, but it is broken
  5. Client drops the object and return an error
  6. The connection is returned to the pool

So in between health checks, a connection is always returned to the pool even if it is broken. There is a mechanism in place in mobc to prevent a connection from re-entering the pool: https://github.com/importcjj/mobc/blob/master/src/lib.rs#L648

Currently the manager always return true (https://github.com/importcjj/mobc/blob/master/src/lib.rs#L187) since it doesn't override the method validate from the Manager trait (https://github.com/prisma/quaint/blob/master/src/pooled/manager.rs#L74). This means we needs to store information in the connection that the manager can read to deny re-entry. Unfortunately the current Queryable interface doesn't take a mutable reference to self so we would need to change that first for all methods (https://github.com/prisma/quaint/blob/master/src/connector/queryable.rs#L19) and then we could add a method is_dead or in_error to that the validate function would call.

I can do the changes, but I wanted to check with the team before since it is significant.

pimeys commented 3 years ago

I'll take a look next sprint. Sounds like a good idea, but the implementation can be a bit hairy.