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

Fix/ssl shutdown #7372

Merged
merged 3 commits into from
Feb 4, 2019
Merged

Fix/ssl shutdown #7372

merged 3 commits into from
Feb 4, 2019

Conversation

bcardiff
Copy link
Member

@bcardiff bcardiff commented Feb 4, 2019

Fix #7364

After #7068 the logic to ignore the OpenSSL::SSL::Error::SYSCALL while closing the connection was lost.

This PR also removes unused code for when Errno was raised.

It adds some manual specs that exhibited issues: request to google and closing the connection before consuming the body. These are domain sensible.

@bcardiff
Copy link
Member Author

bcardiff commented Feb 4, 2019

FYI the other manual specs includes badssl_spec.cr, but it's been a long time since they were added but there were not checked regularly. I've found that connecting to incomplete-chain.badssl.com hangs and the last it fails with

       Closed stream (IO::Error)
         from src/io.cr:128:5 in 'check_open'
         from src/io/buffered.cr:116:5 in 'write'
         from src/openssl/bio.cr:24:7 in '->'
         from BIO_write
         from ssl3_write_pending
         from do_ssl3_write
         from ssl3_write_bytes
         from ssl3_write
         from SSL_write
         from src/openssl/ssl/socket.cr:121:5 in 'unbuffered_write'
         from src/io/buffered.cr:205:5 in 'flush'
         from src/io/buffered.cr:213:5 in 'close'
         from spec/manual/badssl_spec.cr:13:3 in 'connect_to'

cc: @jhass

@bcardiff
Copy link
Member Author

bcardiff commented Feb 4, 2019

I think we can add a make manual_spec target to run this slow specs and we can include them in nightly build or at least in test-ecosystem.

@bcardiff bcardiff requested review from RX14 and ysbaddaden February 4, 2019 16:54
@bcardiff bcardiff added this to the 0.27.2 milestone Feb 4, 2019
@bcardiff bcardiff added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:networking labels Feb 4, 2019
@bcardiff bcardiff merged commit b1abae7 into crystal-lang:master Feb 4, 2019
@bcardiff bcardiff deleted the fix/ssl_shutdown branch February 4, 2019 19:13
bcardiff added a commit that referenced this pull request Feb 4, 2019
* Add manual specs to perform https requests
* Assume connection is closed when ssl_shutdown reports syscall error
Based on d5fe700
* Cleanup unused code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:networking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants