Skip to content
This repository has been archived by the owner on Mar 21, 2020. It is now read-only.

KEY-56 - Checkout do_ssl_handshake error and close if fatal #93

Merged
merged 2 commits into from
Dec 15, 2014

Conversation

jgrahamc
Copy link
Contributor

@jgrahamc jgrahamc commented Dec 9, 2014

Previously, the first call to do_ssl_handshake was assume to 'work'
(i.e. return WANT_READ or WANT_WRITE) and the error code
was not checked. Added checking of the error code and if it is not
retriable then drop the connection and log the error.

@grittygrease
Copy link
Contributor

LGTM

@PiotrSikora
Copy link
Contributor

Shouldn't we also add logging for the other do_ssl_handshake() call?

@jgrahamc
Copy link
Contributor Author

Uh. Yes, we should. I thought it was being logged but alas the error is just being cleared and the connection terminated. I will update this pull request.

Previously, the first call to do_ssl_handshake was assume to 'work'
(i.e. return WANT_READ or WANT_WRITE) and the error code
was not checked. Added checking of the error code and if it is not
retriable then drop the connection and log the error.
@jgrahamc
Copy link
Contributor Author

Updated.

@PiotrSikora
Copy link
Contributor

Looks good, feel free to merge it.

Previously, connection_terminate handled SSL_shutdown return codes
poorly and assumed that SSL_shutdown would work (it retried it
once). This could lead to a situation where SSL_shutdown returned
WANT_READ or WANT_WRITE which would be ignored and then the TCP
connection terminated.

Now each connection has a TERMINATING state added and it will
continue to process reads and call SSL_shutdown repeatedly on read
events until the connection has been terminated. Only then will
memory be cleaned up and the TCP connection killed.
grittygrease added a commit that referenced this pull request Dec 15, 2014
KEY-56 - Checkout do_ssl_handshake error and close if fatal
@grittygrease grittygrease merged commit 7d47439 into cloudflare:master Dec 15, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants