Fix leakage of promises by MahdiBM · Pull Request #497 · vapor/postgres-nio

MahdiBM · 2024-08-03T11:44:11Z

resolves #496

codecov · 2024-08-03T11:47:14Z

Codecov Report

Attention: Patch coverage is 80.50847% with 23 lines in your changes missing coverage. Please review.

Project coverage is 55.17%. Comparing base (9f84290) to head (892ef0b).

Files with missing lines	Patch %	Lines
...urces/PostgresNIO/New/PostgresChannelHandler.swift	82.55%	15 Missing ⚠️
...nection State Machine/ConnectionStateMachine.swift	75.00%	4 Missing ⚠️
...w/Connection State Machine/CloseStateMachine.swift	0.00%	2 Missing ⚠️
Sources/PostgresNIO/New/NotificationListener.swift	80.00%	1 Missing ⚠️
Sources/PostgresNIO/New/PSQLTask.swift	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #497      +/-   ##
==========================================
+ Coverage   55.12%   55.17%   +0.05%     
==========================================
  Files         127      127              
  Lines       10174    10207      +33     
==========================================
+ Hits         5608     5632      +24     
- Misses       4566     4575       +9

Files with missing lines	Coverage Δ
...tion State Machine/ExtendedQueryStateMachine.swift	`77.17% <100.00%> (+0.06%)`	⬆️
Sources/PostgresNIO/New/NotificationListener.swift	`69.87% <80.00%> (+0.74%)`	⬆️
Sources/PostgresNIO/New/PSQLTask.swift	`75.67% <75.00%> (-1.47%)`	⬇️
...w/Connection State Machine/CloseStateMachine.swift	`0.00% <0.00%> (ø)`
...nection State Machine/ConnectionStateMachine.swift	`61.18% <75.00%> (-0.03%)`	⬇️
...urces/PostgresNIO/New/PostgresChannelHandler.swift	`83.72% <82.55%> (+0.04%)`	⬆️

... and 1 file with indirect coverage changes

MahdiBM · 2024-08-03T12:33:13Z

Not sure how exactly add a test for this.
something like:

        let options = XCTMeasureOptions.default
        options.iterationCount = 100
        measure(metrics: [XCTMemoryMetric()], options: options) {
            do {
                let conn = try PostgresConnection.test(on: self.eventLoop).wait()
                _ = try conn.query(
                    "SELECT current_setting('application_name')",
                    logger: .psqlNoOpLogger
                ).wait()
                try conn.close().wait()
            } catch {
                XCTFail(String(reflecting: error))
            }
        }

could work but it's too integrated into Xcode.

Looks like a case that needs benchmarking, like with package-benchmark.

Sources/PostgresNIO/New/Connection State Machine/ConnectionStateMachine.swift

Tests/PostgresNIOTests/New/Data/Date+PSQLCodableTests.swift

Sources/PostgresNIO/New/PostgresChannelHandler.swift

MahdiBM · 2024-08-05T22:03:30Z

@fabianfett another round of review please 🙂

Sources/PostgresNIO/New/PostgresChannelHandler.swift

Sources/PostgresNIO/New/Connection State Machine/AuthenticationStateMachine.swift

MahdiBM · 2024-08-08T11:09:41Z

Sources/PostgresNIO/New/Connection State Machine/ConnectionStateMachine.swift

            }
-        case .closeCommand(let closeContext):
+        case .closeCommand(let closeContext, let writePromise):
+            writePromise?.fail(psqlErrror) /// Use `cleanupContext` or not?


Sources/PostgresNIO/New/Connection State Machine/ConnectionStateMachine.swift

MahdiBM · 2024-08-08T11:18:17Z

Sources/PostgresNIO/New/PostgresChannelHandler.swift

        case .failure(let error):
            let action = self.state.errorHappened(.unlistenError(underlying: error))
            self.run(action, with: context)
+            writePromise?.fail(error) /// Should I pass the promise to the action? seemed troublesome


MahdiBM · 2024-08-08T11:18:46Z

Sources/PostgresNIO/New/PostgresChannelHandler.swift

                ),
-                logger: preparedStatement.logger,
-                promise: preparedStatement.promise
+                writePromise: nil // Ignore


Sources/PostgresNIO/New/Connection State Machine/AuthenticationStateMachine.swift

Sources/PostgresNIO/New/Connection State Machine/ConnectionStateMachine.swift

MahdiBM · 2024-08-08T13:29:01Z

Sources/PostgresNIO/New/Connection State Machine/ConnectionStateMachine.swift

        switch task {
-        case .extendedQuery(let queryContext):
+        case .extendedQuery(let queryContext, let writePromise):
+            writePromise?.fail(psqlErrror) /// Use `cleanupContext` or not?


MahdiBM · 2024-08-08T13:29:34Z

Sources/PostgresNIO/New/Connection State Machine/ExtendedQueryStateMachine.swift

+                return .sendBindExecuteSync(prepared, promise: promise)
            }

+            /// Not my code, but this is ignoring the last argument which is a promise? is that fine?


MahdiBM · 2024-08-08T13:37:45Z

Sources/PostgresNIO/New/PostgresChannelHandler.swift

+                    list.startListeningSucceeded(handler: self, writePromise: nil)
                }
-
+                writePromise?.succeed(()) /// Should we instead do smth like "whenAllSucceed"?


MahdiBM marked this pull request as ready for review August 3, 2024 15:31

MahdiBM requested review from fabianfett and gwynne as code owners August 3, 2024 15:31

MahdiBM commented Aug 3, 2024

View reviewed changes

Sources/PostgresNIO/New/Connection State Machine/ConnectionStateMachine.swift Outdated Show resolved Hide resolved

MahdiBM commented Aug 3, 2024

View reviewed changes

Tests/PostgresNIOTests/New/Data/Date+PSQLCodableTests.swift Outdated Show resolved Hide resolved

MahdiBM commented Aug 3, 2024

View reviewed changes