vapor / postgres-nio

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

Memory leak in version 1.22.0 #496

Open rausnitz opened 3 months ago

rausnitz commented 3 months ago

Describe the issue

Memory usage spiked when we deployed our application with postgres-nio 1.22.0.

Vapor version

4.102.1

Operating system and version

Linux

Swift version

Swift 5.10

Steps to reproduce

Run a Swift backend application that uses postgres-nio and compare the memory usage when on 1.21.5 and 1.22.0.

Outcome

Memory usage steadily increases after starting an application that uses postgres-nio 1.22.0.

Additional notes

We observed this on an instance that runs recurring jobs but does not receive web traffic, so memory usage tends to be stable over time.

In the attached screenshot, the first release to include postgres-nio 1.22.0 was on July 26. The sudden drops in memory usage indicate when new releases went live, restarting the app. The graph stabilizes on July 30 when we reverted postgres-nio to 1.21.5.

postgres-nio-memory

Edit: I'm not sure if the memory usage spikes in 1.21.6 or 1.22.0.

MahdiBM commented 3 months ago

Repro (No need to even do a query):

@main
struct EntryPoint {

    static func main() async throws {
        let config = PostgresClient.Configuration(...)
        let client = PostgresClient(configuration: config)

        try await withThrowingTaskGroup(of: Void.self) { taskGroup in
            taskGroup.addTask {
                await client.run()
            }

            taskGroup.addTask {
                for idx in (0..<100_000) {
                    if idx % 10 == 0 {
                        print(idx)
                    }
                    try await Task.sleep(for: .milliseconds(10))
                    try await client.withConnection { conn in
                        /// No need to even do a query
                    }
                }
            }

            try await taskGroup.waitForAll()
        }
    }
}
MahdiBM commented 3 months ago

No need to even be able to create a proper connection?:

@main
struct EntryPoint {
    static func main() async throws {
        let config = PostgresClient.Configuration(
            host: "CORRECT-HOST",
            port: 5432,
            username: "USER",
            password: "BAD_PASSWORD",
            database: "DB",
            tls: .disable
        )

        let client = PostgresClient(configuration: config)

        try await withThrowingTaskGroup(of: Void.self) { taskGroup in
            taskGroup.addTask {
                await client.run()
            }

            taskGroup.addTask {
                for idx in (0..<100_000) {
                    if idx % 10 == 0 {
                        print(idx)
                    }
                    try await Task.sleep(for: .milliseconds(10))
                    try await client.withConnection { conn in

                    }
                }
            }

            try await taskGroup.waitForAll()
        }
    }
}
petrpavlik commented 3 months ago

I'll just add that memory leak instruments in Xcode is reporting a leak when profiling my backend code.

Screenshot_2024-07-31_at_22 38 41
fabianfett commented 3 months ago

Okay, found the source of this leak:

With 1.22.0 we started to forward write promises:

https://github.com/vapor/postgres-nio/blob/d18b137640222fe29a22568077c4799d213fdf96/Sources/PostgresNIO/Connection/PostgresConnection.swift#L545

However in the success case we never forward them to the wire in the PostgresChannelHandler.

https://github.com/vapor/postgres-nio/blob/d18b137640222fe29a22568077c4799d213fdf96/Sources/PostgresNIO/New/PostgresChannelHandler.swift#L199

petrpavlik commented 2 months ago

Would it maybe make sense to revert the commit that has caused the leak before the fix PR is approved and merged in?

LukasCZ commented 2 months ago

Any update on this? This likely affects vast majority of web servers written in Vapor, and it's been broken for three weeks now. When can we expect it to be fixed and merged in? Or should I pin to the previous version of postgres-nio for now?

Screenshot 2024-08-20 at 13 18 40
fabianfett commented 2 months ago

@LukasCZ thanks for pinging. Will prepare a revert pr for now.

fabianfett commented 2 months ago

PR is up: https://github.com/vapor/postgres-nio/pull/501

fabianfett commented 2 months ago

Released as 1.22.1

LukasCZ commented 2 months ago

Thank you @fabianfett 🙏