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

Avoid calling handleClientConnection if initializeClientConnection fails #221

Merged
merged 1 commit into from
Oct 13, 2017

Conversation

djones6
Copy link
Contributor

@djones6 djones6 commented Oct 13, 2017

Description

This is a follow-on to #219, which split out the socket accept from the protocol initialization for SSL connections, in order to perform the latter on a separate thread.

In the case where the socket initialization fails, such as with an SSL handshake error, following #219 we would log the error but then attempt to handle the client connection anyway, resulting in unnecessary work and further errors.

This PR moves the do/catch logic for initializeClientConnection so that we only try to call handleClientConnection if the former was successful.

I also updated the log messages to document the source of a connection when it fails initialization.

Motivation and Context

Avoids unnecessary attempts to handle a connection for which the SSL initialization has failed, and reporting of further errors as a consequence.

How Has This Been Tested?

I have run the Kitura-net tests and they are passing. Note that there is an intermittent test failure exposed by the test intrduced in #219 - which intentionally generates a bad connection to an SSL listener - which will be resolved by Kitura/BlueSSLService#42.

Checklist:

  • I have submitted a CLA form
  • If applicable, I have updated the documentation accordingly.
  • If applicable, I have added tests to cover my changes.

@codecov-io
Copy link

codecov-io commented Oct 13, 2017

Codecov Report

Merging #221 into master will decrease coverage by 0.27%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #221      +/-   ##
==========================================
- Coverage   74.02%   73.75%   -0.28%     
==========================================
  Files          31       31              
  Lines        4146     4145       -1     
==========================================
- Hits         3069     3057      -12     
- Misses       1077     1088      +11
Flag Coverage Δ
#CHTTPParser 56.11% <ø> (ø) ⬆️
#KituraNet 73.75% <70%> (-0.28%) ⬇️
Impacted Files Coverage Δ
Sources/KituraNet/HTTP/HTTPServer.swift 79.09% <70%> (-1.81%) ⬇️
Sources/KituraNet/IncomingSocketManager.swift 85.45% <0%> (-5.46%) ⬇️
...ces/KituraNet/Server/ServerLifecycleListener.swift 79.16% <0%> (-4.17%) ⬇️
Sources/KituraNet/IncomingSocketHandler.swift 76.99% <0%> (-1.88%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3b3c5d...c173d65. Read the comment docs.

@djones6
Copy link
Contributor Author

djones6 commented Oct 13, 2017

I'm accepting the apparent reduction in coverage - this is really because of a fix I added to BlueSSLService which no longer (erroneously) triggers an error handling path. Raised #222 for a proper test for this path.

@djones6 djones6 merged commit 52a9abc into master Oct 13, 2017
@djones6 djones6 deleted the handleInitErrors branch October 13, 2017 14:18
Copy link
Collaborator

@tunniclm tunniclm left a comment

Choose a reason for hiding this comment

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

lgtm

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