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

Suggestions for avoiding issues similar to #1172 and #1104 #1176

Open
cmcaine opened this issue May 9, 2024 · 0 comments
Open

Suggestions for avoiding issues similar to #1172 and #1104 #1176

cmcaine opened this issue May 9, 2024 · 0 comments

Comments

@cmcaine
Copy link
Contributor

cmcaine commented May 9, 2024

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
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

No branches or pull requests

1 participant