Skip to content
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

Use sync pipeline operations to add NIOSSLClientHandler #152

Merged
merged 1 commit into from
May 11, 2021

Conversation

fabianfett
Copy link
Collaborator

  • Bump required NIO dependencies.
  • Modify the PSQLChannelHandler's enableSSLCallback to a synchronous API
  • Use channel.pipeline.syncOperations to add the NIOSSLClientHandler after it was verified that SSL is supported on this connection
  • Extend test testEstablishSSLCallbackIsCalledIfSSLIsSupported to ensure startup is send after SSL connection is established.

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

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

LGTM. Will let @gwynne have the final say. Changed to patch update since there are no public API additions

@fabianfett
Copy link
Collaborator Author

@0xTim Agree there are no api changes in Postgres, but we bump or dependencies, that we in part reexport (like NIO) in a minor way. Therefor this needs IMO a minor release.

@0xTim
Copy link
Member

0xTim commented May 1, 2021

Oh FFS why is PostgresNIO exporting everything! * sigh * you're right I'll change it back. I can't wait to get rid of all these exports!

@fabianfett
Copy link
Collaborator Author

We'll it's not so much export everything. But in this case EventLoopFuture is enough. 😎

Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -215,17 +215,16 @@ final class PSQLConnection {
.channelInitializer { channel in
let decoder = ByteToMessageHandler(PSQLBackendMessage.Decoder())

var enableSSLCallback: ((Channel) -> EventLoopFuture<Void>)? = nil
var enableSSLCallback: ((Channel) throws -> ())? = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit but this might be clearer as configureSSLCallback

}.flatMap { sslHandler in
channel.pipeline.addHandler(sslHandler, position: .before(decoder))
}
channel.eventLoop.preconditionInEventLoop()
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW the sync operations will do this check for you; keeping this (and potentially degrading it to an assertion) is a useful as an indicator to whoever looks at this code though.

@fabianfett fabianfett force-pushed the ff-use-sync-pipeline-operations branch from 0e0caf4 to 707b9e7 Compare May 10, 2021 07:59
@fabianfett
Copy link
Collaborator Author

Renamed enableSSLCallback to configureSSLCallback.

@0xTim 0xTim merged commit 6629d63 into vapor:main May 11, 2021
@fabianfett fabianfett deleted the ff-use-sync-pipeline-operations branch May 11, 2021 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants