Skip to content

Fix leakage of promises #497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Fix leakage of promises #497

wants to merge 4 commits into from

Conversation

@MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Aug 3, 2024

resolves #496

@codecov
Copy link

codecov bot commented Aug 3, 2024 •
edited
Loading

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
Copy link
Contributor Author

MahdiBM commented Aug 3, 2024 •
edited
Loading

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.

@MahdiBM MahdiBM marked this pull request as ready for review August 3, 2024 15:31
MahdiBM
MahdiBM commented Aug 3, 2024
View reviewed changes
MahdiBM
MahdiBM commented Aug 3, 2024
View reviewed changes
fabianfett
fabianfett reviewed Aug 5, 2024
View reviewed changes
fabianfett
fabianfett reviewed Aug 5, 2024
View reviewed changes
MahdiBM
MahdiBM commented Aug 5, 2024
View reviewed changes
MahdiBM
MahdiBM commented Aug 5, 2024
View reviewed changes
MahdiBM
MahdiBM commented Aug 5, 2024
View reviewed changes
MahdiBM
MahdiBM commented Aug 5, 2024
View reviewed changes
MahdiBM
MahdiBM commented Aug 5, 2024
View reviewed changes
MahdiBM
MahdiBM commented Aug 5, 2024
View reviewed changes
MahdiBM
MahdiBM commented Aug 5, 2024
View reviewed changes
@MahdiBM MahdiBM requested a review from fabianfett August 5, 2024 22:02
@MahdiBM
Copy link
Contributor Author

MahdiBM commented Aug 5, 2024

@fabianfett another round of review please 🙂

fabianfett
fabianfett requested changes Aug 6, 2024
View reviewed changes
@MahdiBM MahdiBM force-pushed the mmbm-fix-promise-leak branch 2 times, most recently from afc12db to 7aaae6e Compare August 8, 2024 10:48
MahdiBM
MahdiBM commented Aug 8, 2024
View reviewed changes
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?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

MahdiBM
MahdiBM commented Aug 8, 2024
View reviewed changes
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

MahdiBM
MahdiBM commented Aug 8, 2024
View reviewed changes
Sources/PostgresNIO/New/PostgresChannelHandler.swift
),
logger: preparedStatement.logger,
promise: preparedStatement.promise
writePromise: nil // Ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignore?

@MahdiBM MahdiBM requested a review from fabianfett August 8, 2024 11:19
fabianfett
fabianfett reviewed Aug 8, 2024
View reviewed changes
MahdiBM
MahdiBM commented Aug 8, 2024
View reviewed changes
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?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

MahdiBM
MahdiBM commented Aug 8, 2024
View reviewed changes
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?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

MahdiBM
MahdiBM commented Aug 8, 2024
View reviewed changes
Sources/PostgresNIO/New/PostgresChannelHandler.swift
list.startListeningSucceeded(handler: self, writePromise: nil)
}

writePromise?.succeed(()) /// Should we instead do smth like "whenAllSucceed"?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

@MahdiBM MahdiBM force-pushed the mmbm-fix-promise-leak branch from b966180 to 8b944b2 Compare August 8, 2024 13:40
@MahdiBM MahdiBM requested a review from fabianfett August 8, 2024 13:41
@MahdiBM MahdiBM mentioned this pull request Aug 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gwynne gwynne Awaiting requested review from gwynne gwynne is a code owner

@fabianfett fabianfett Awaiting requested review from fabianfett fabianfett is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memory leak in version 1.22.0

2 participants

@MahdiBM @fabianfett