Skip to content

Leaking promise created at ConnectionPool.swift line 373 #179

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.

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

Closed
fabianfett opened this issue Mar 9, 2020 · 9 comments · Fixed by #180
Closed

Leaking promise created at ConnectionPool.swift line 373 #179

fabianfett opened this issue Mar 9, 2020 · 9 comments · Fixed by #180
Labels
kind/bug Feature doesn't work as expected.
Milestone

Comments

@fabianfett
Copy link
Member

Hi,

I just checked out the current master commit 9cdd1dc and wanted to test it with my lambda-runtime.

When building for macOS in DEBUG, I ran immediately into a fatalError:

Fatal error: leaking promise created at (file: "/Users/xxx/Library/Developer/Xcode/DerivedData/xxx-brfbdomqotikxpeqafasrajmegus/SourcePackages/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool.swift", line: 373): file /Users/xxx/Library/Developer/Xcode/DerivedData/xxx-brfbdomqotikxpeqafasrajmegus/SourcePackages/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool.swift, line 373

This happens on the first invocation of execute.

I haven't found a similar issue here, so that's why I created one.

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 9, 2020

Hmm, this error could well be caused by a path where promises can be leaked if creating NIOSSLContext objects fail. Specifically:

do {
let tlsConfiguration = tlsConfiguration ?? TLSConfiguration.forClient()
let context = try NIOSSLContext(configuration: tlsConfiguration)
let handlers: [ChannelHandler] = [
try NIOSSLClientHandler(context: context, serverHostname: key.host.isIPAddress ? nil : key.host),
TLSEventsHandler(completionPromise: handshakePromise),
]
return self.addHandlers(handlers)
} catch {
return self.eventLoop.makeFailedFuture(error)
}

handshakePromise is passed to TLSEventsHandler here, but if the line above throws then it'll be leaked.

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 9, 2020

(The same logic applies to the return value of self.addHandlers which can also leak this promise if the returned future fails)

@fabianfett
Copy link
Member Author

@Lukasa For reference I create a connection to http://localhost:7000.

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 9, 2020

Heh, yup, that promise can also be leaked if the connection attempt fails.

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 9, 2020

Lots of ways to leak that promise it seems.

@fabianfett
Copy link
Member Author

Okay, besides the fact, that we shouldn't leak I found the issue, why it was happening to me. I tried to connect to http://http://127.0.0.1:7000. So I guess dns failed?

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 9, 2020

Yup, that sounds right.

@weissi
Copy link
Contributor

weissi commented Mar 11, 2020

CC @adtrevor

@weissi weissi added the kind/bug Feature doesn't work as expected. label Mar 11, 2020
@weissi weissi added this to the 1.2.0 milestone Mar 11, 2020
@PopFlamingo
Copy link
Contributor

@fabianfett Thank you for reporting the issue, #180 should fix it 🙂
cc: @Lukasa @weissi

weissi pushed a commit that referenced this issue Mar 12, 2020
motivation:
TLS handshake promise was leaked in some cases of failure (see #179)

changes:
- Avoid leaking promise
- Clearer completion flow for related futures
- Add testAvoidLeakingTLSHandshakeCompletionPromise test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Feature doesn't work as expected.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants