Skip to content

Make testSSLHandshakeErrorPropagation less flaky #380

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

Conversation

fabianfett
Copy link
Member

Motivation

  • The testSSLHandshakeErrorPropagation has a very short connect timeout (100ms). The reason for this is, that a failing TLS handshake manifest in NIOTS as a connect timeout and we don’t want to wait for the timeout for to long in our tests.
  • When using NIOSSL however, we expect a proper NIOSSLError. Sometimes though the connection is not setup fast enough (because of the very short 100ms connect timeout), that we get a connectTimeout error instead. See build logs: https://ci.swiftserver.group/job/async-http-client-swift52-prb/480/console
    Test Case 'HTTPClientTests.testSSLHandshakeErrorPropagation' started at 2021-06-22 19:14:49.426
    /code/Tests/AsyncHTTPClientTests/HTTPClientTests.swift:2735: error: 
    HTTPClientTests.testSSLHandshakeErrorPropagation : failed - Handshake failed with unexpected error: connectTimeout(NIO.TimeAmount(nanoseconds: 100000000))
    

Modifications

  • Setting the short connect timeout to 100ms when using NIOTS only. This will make sure we get the proper NIOSSLError when using NIOSSL

@fabianfett fabianfett added the semver/none No version bump required. label Jun 23, 2021
### Motivation

- The `testSSLHandshakeErrorPropagation` has a very short connect timeout (100ms). The reason for this is, that a failing TLS handshake manifest in NIOTS as a connect timeout and we don’t want to wait for the timeout for to long in our tests.
- Using a the NIOSSL however we expect a proper NIOSSLError. Sometimes though the connection is not setup fast enough (because of the very short 100ms connect timeout), that we get a connectionTimeout error instead.

### Modifications

- Setting the short connect timeout to 100ms when using NIOTS only. This will make sure we get the proper NIOSSLError when using NIOSSL
@fabianfett fabianfett force-pushed the ff-testSSLHandshakeErrorPropagation-less-flaky branch from 165edea to 7c7717e Compare June 23, 2021 08:00
Copy link
Collaborator

@Davidde94 Davidde94 left a comment

Choose a reason for hiding this comment

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

Nice!

@Davidde94 Davidde94 merged commit 132dc3e into swift-server:main Jun 23, 2021
@fabianfett fabianfett deleted the ff-testSSLHandshakeErrorPropagation-less-flaky branch July 10, 2021 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants