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

Refactor - HTTPServer upgrade handler #88

Merged
merged 2 commits into from
Sep 7, 2018

Conversation

nethraravindran
Copy link
Contributor

@nethraravindran nethraravindran commented Sep 6, 2018

Refactored the code around KituraWebSocketUpgrader to improve readability

@@ -68,6 +68,8 @@ public class HTTPServer : Server {
/// The event loop group on which the HTTP handler runs
let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: System.coreCount)

private var channelHandlerCtx: ChannelHandlerContext?
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering SwiftNIO conventions, simply calling this ctx may make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes :) calling ctx looks good

Copy link
Contributor

@pushkarnk pushkarnk left a comment

Choose a reason for hiding this comment

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

LGTM, just a small comment inline

@pushkarnk
Copy link
Contributor

@nethraravindran You may also want to run tests from the WebSocket NIO port locally, before we merge this.

@nethraravindran
Copy link
Contributor Author

nethraravindran commented Sep 7, 2018

Few WebSocket tests failed with #86
Incorrect HTTPVersion in sendResponse(channel: handler: status: withBody) were the reasons for those failures. Defaulting to 1 fixes the issue.

@nethraravindran
Copy link
Contributor Author

nethraravindran commented Sep 7, 2018

@pushkarnk Thanks! I have run WebSocket tests and all the tests are successful with this change 97772c6

@pushkarnk
Copy link
Contributor

Thanks!

@pushkarnk pushkarnk merged commit e96838b into Kitura:master Sep 7, 2018
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.

2 participants