`AuthenticationSessionsMiddleware` should use a connection pool

[MrMage]

At the moment, AuthenticationSessionsMiddleware calls A.authenticate(sessionID: aID, on: req), which reserves a database connection for the lifetime of the request. For long-running requests that only use pooled database connections elsewhere, this can cause DB connection count bottlenecks with e.g. Postgres. This problem is exacerbated by the fact that AuthenticationSessionsMiddleware might run on every request. (Postgres has very strict connection limits by default.)

In fact, the following has happened to me in production:

My database connection pool size is 3. I send three concurrent requests to the server. The middleware flow is like this:

1. SessionsMiddleware is invoked and in turn calls DatabaseKeyedCache.get, which uses a connection pool.
2. AuthenticationSessionsMiddleware authenticates the user and reserves a database connection for the lifetime of the request.
3. After the request has finished, SessionsMiddleware calls DatabaseKeyedCache.set in addCookies, again using a connection pool.

Now, if all three database connections are taken by AuthenticationSessionsMiddleware after step 2, requesting another connection from the pool will block (because the pool has been exhausted by the cached connections), resulting in a deadlock on all three requests. Especially if DB calls have non-negligible latency (e.g. 50-100ms) due to the DB and the Vapor server not running on the same machine, this can be problematic, even with slightly higher connection pool sizes.

To solve this, I would suggest using a pooled connection for authenticating the user. This might make changes to SessionAuthenticatable necessary, though.

[tanner0101]

Yeah, this is unfortunately a big problem with Vapor 3's design. See here for additional discussion: https://github.com/vapor/vapor/issues/1711

Vapor 4's design is exactly what you described: anything that needs connections, whether it be a middleware or a controller will get a pool. Each query to the DB will pop a connection from that pool for the lifetime of the query. This will allow a max connection pool size of 1 to work without causing deadlocks.

As for porting this fix back to Vapor 3, I'm not sure it's worth the potential issues it could cause. I see this as more of a fundamental design flaw than a bug.

[tanner0101]

See also: https://github.com/vapor/fluent/issues/564#issuecomment-416355762

[MrMage]

Vapor 4's design is exactly what you described: anything that needs connections, whether it be a middleware or a controller will get a pool. Each query to the DB will pop a connection from that pool for the lifetime of the query. This will allow a max connection pool size of 1 to work without causing deadlocks.

That sounds great! Would you mind providing a pointer to how this is going to be implemented (or is implemented) in Fluent 4? In particular, how does the pool know when the query is finished? For something like db.withPooledConnection { connection in query(on: connection) }, this is obvious, but how would

let db = try container.connectionPool(for: .sqlite)
router.get("work") { req -> String in
    _ = Todo.query(on: db).all().map { todos in
        print(todos)
    }
    return "done"
}

work with regards to detecting when the query is done?

As for porting this fix back to Vapor 3, I'm not sure it's worth the potential issues it could cause. I see this as more of a fundamental design flaw than a bug.

Agreed. I guess we can close this issue, but I'd still like to have a look at the Fluent 4 pool implementation :-)

[tanner0101]

@MrMage see here: https://github.com/vapor/fluent-postgresql/blob/master/Sources/FluentPostgresDriver/Postgres%2BFluent.swift#L35

ConnectionPool conforms to Fluent.Database using withConnection(_:).

[MrMage]

ConnectionPool conforms to Fluent.Database using withConnection(_:).

Looks great! I guess in theory the connection could even be released before onOutput is called; is that the case?

[tanner0101]

Theoretically yes, but in reality NIOMySQL and NIOPostgres don't complete their return futures until they have passed back all rows to the user.

[mxcl]

So… for those of us are experiencing this and who are building something where we anticipate heavy usage in the near future, we should opt in to beta test Vapor 4?

[jdmcd]

@mxcl Depends on your database setup mostly. If you can have a pool size per loop greater than the potential deadlock minimum, then you're ok. Otherwise you should either generate a database connection manually (instead of passing in Request as the DatabaseConnectable object, since that uses request caching) or try out Vapor 4 (Forewarning, I have no idea what the state of the alphas/betas are, I don't think they're even out yet)

[mxcl]

I appreciate the advice, thank you.

[jdmcd]

No problem, let me know if I can help in any way!