vapor / postgres-nio

🐘 Non-blocking, event-driven Swift client for PostgreSQL.
https://api.vapor.codes/postgresnio/documentation/postgresnio/
MIT License
321 stars 75 forks source link

Document how to close connections when returning Futures #130

Open rnikander opened 3 years ago

rnikander commented 3 years ago

I've been using this package and I keep running into a confusing situation. All your examples use the async API in a sync way, by calling wait. Eg,

let db = getConnection()
defer { try! db.close.wait() }
let results = db.query(...).wait()
... etc ...

But what if I want to keep things asynchronous and write functions that return Futures? If you do that you can't close the connection when you leave the scope, like the example code above. I've tried things like:

func getStuff() -> Fut<Stuff> {
    let db = getConnection()
    let fut = db.query("select * from stuff").map { res in 
        makeStuffFromResult(res)
    }
    fut.whenComplete { let _ = db.close() }
    return fut
}

But it looks kind of ugly and I'm not sure it's correct. I think it would help if the documentation contained an explanation of how to properly do this.

neilt commented 2 years ago

I support this also.

As a new user trying to figure out how to use postgres-nio, db.query(...).wait() had absolutely no value for me. I know you want the examples dead simple to try to attract users. But completely useless examples are not any help. For the most part no one is going to be investigating Postgres-nio for use in synchronous code.

Now if there were good examples elsewhere, that would be fine, but I could not find any outside of high level Vapor examples and I am not using Vapor yet. I wan't to know how things work at a lower level.

I assume that when the connection is deallocated it is automatically handled correctly. But who knows?

fabianfett commented 2 years ago

I assume that when the connection is deallocated it is automatically handled correctly. But who knows?

Sadly this is not true. I know that EventLoopFutures are hard to understand at first. Since they are a fundamental building block for all of swift on server they are not explained here.

However the real solution to this problem is: Drop using EventLoopFutures and move to an async/await interface, which is what I'm currently working on... Do you think this would solve your problem?

neilt commented 2 years ago

Well it depends on how the connection is handled.

It is entirely unclear to me the proper way to handle the connection after reading the original post.

For example, given:

let gPostgresPools = EventLoopGroupConnectionPool(source: PostgresConnectionSource(configuration: gPostgresConfiguration),
                                                  maxConnectionsPerEventLoop: 2,
                                                  logger: gPostgresLogger,
                                                  on: gPostgresEventLoopGroup)

class SomeClass {
    let db: PostgresDatabase
    init() {
        db = gPostgresPools.database(logger: gPostgresLogger)
    }
    func xx() -> Future {
        let future = db.query(....).flatMap { .... }
        return future
    }
}
  1. Now does the code have any responsibility to clean up db other than let it go out of scope?
  2. Should the code hold db like this? If so will it have performance or other impacts?
  3. Is getting db from the pool quick enough that the code should just get it from the pool each time the code is going to execute SQL?
  4. Are you saying that the code should be changed to something like (untested) :
func xx() async -> [SomeValue] {
   do {
        return try await db.query(....).map { .... }.get()
    } catch { ... }
}

@fabianfett To answer your question async/await works fine as in the last example, although I have not tested at scale. It is either try/catch with async/await or future.whenSuccess/whenFailure with futures. It seems async/await is easier to understand.

@fabianfett Not sure I understand enough about postgres-nio to understand what you are working on.