You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.jlstructOpenSSLSocket(sslconfig) <:AbstractTLSSocket
sslconfig::OpenSSL.SSLContextendOpenSSLSocket() =OpenSSLSocket(default_ssl_config)
# In src/Connections.jlfunctionsslupgrade(socket_specification::AbstractTLSSocket, ...)
endfunctionsslconnection(socket_specification::OpenSSLSocket, ...)
endfunctionsslconnection(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
The text was updated successfully, but these errors were encountered:
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
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
socket_type_tls
, instead deriving it from sslconfigHTTP.TLSConfig(engine=:openssl, some, other, kw, options, here)
andTLSConfig
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:
Any of these might have detected #1172 before the merge:
HTTP.request
would have caught itAnd these things might have meant the original regression from the change in meaning of
sslconfig
(#1106) was caught:HTTP.request
The text was updated successfully, but these errors were encountered: