Suggestions for avoiding issues similar to #1172 and #1104

[cmcaine]

Sorry for the hassle of introducing a bug (#1172) with #1106 (my PR to fix #1104). In the interests of avoiding similar errors in HTTP.jl or HTTP2.jl, here's a reflection on what happened.

Conclusion first: my suggestions for future versions of this package or HTTP2.jl

• If a parameter depends on the value of another, bundle them together in a struct or similar
• More parametric tests and invest in test fixtures/mocks/implementations of stuff that's needed for them (e.g. proxies)

I made the original PR (#1106) ages ago so I can't remember what I was thinking at the time, but I probably thought connectionlayer() was too confusing to try to understand or I half arsed it by getting my tests to pass and trusted to my competence or review to catch any errors.

These changes to the API would probably have prevented #1104:

• A change to the interface for specifying the TLS config: anything that bundles socket_type_tls and sslconfig together

  • remove keyword option socket_type_tls, instead deriving it from sslconfig
  • something like this?

  # In HTTP.jl or changed slightly to be in OpenSSL.jl and MbedTLS.jl
  struct OpenSSLSocket(sslconfig) <: AbstractTLSSocket
  sslconfig::OpenSSL.SSLContext
  end
  
  OpenSSLSocket() = OpenSSLSocket(default_ssl_config)
  
  # In src/Connections.jl
  
  function sslupgrade(socket_specification::AbstractTLSSocket, ...)
  end
  
  function sslconnection(socket_specification::OpenSSLSocket, ...)
  end
  
  function sslconnection(socket_specification::MbedTLSSocket, ...)
  end

  • or some interface like HTTP.TLSConfig(engine=:openssl, some, other, kw, options, here) and TLSConfig is responsible for making an MbedTLS or OpenSSL config with the right settings.

And these things might have reduced the chance of #1104 occurring in the first place:

• if connectionlayer() was simpler
• if some version of sockettype() was used in both places in connectionlayer() where a sockettype is chosen

Any of these might have detected #1172 before the merge:

• I, or a reviewer, could have grepped for each use of each variable whose semantics I had changed
• The nested TLS socket behaviour when proxying could have had tests
• Decent fuzz or parametric tests of HTTP.request would have caught it
• If the existing proxy test tested a functional proxy rather than a non-functional one then that might have caught the issue

And these things might have meant the original regression from the change in meaning of sslconfig (#1106) was caught:

• Test coverage of the sslconfig option to HTTP.request