Fix NameError in async-http adapter on protocol-http1 >= 0.40 - #1130
Conversation
| # status description. | ||
| let(:server_url) { "http://#{WebMockServer.instance.host_with_port}/" } | ||
|
|
||
| after { WebMock.reset_callbacks } |
There was a problem hiding this comment.
WebMock.allow_net_connect! leaks to subsequent examples, since WebMock.reset! does not restore the net connect setting. Can you restore it in the after hook?
| after { WebMock.reset_callbacks } | |
| after do | |
| WebMock.reset_callbacks | |
| WebMock.disable_net_connect! | |
| end |
There was a problem hiding this comment.
Good catch, thanks. WebMock.reset! only clears the stub and request registries, so the config flag survives the example. Applied your suggestion.
There was a problem hiding this comment.
@koic Worth flagging that the same leak already exists elsewhere: the two callback examples just above this one both call allow_net_connect! with nothing restoring it, and so does complex_cross_concern_behaviors.rb:3, which runs for every adapter. Nothing visibly breaks today, since the examples that follow are either stubbed or re-enter a context that disables it. Leaving it out of this PR, but flagging it in case you think it is worth a look.
protocol-http1 0.40.0 removed Protocol::HTTP1::Reason in favour of
Protocol::HTTP::Status, but the async-http adapter still reads
Protocol::HTTP1::Reason::DESCRIPTIONS when building a WebMock response.
Any real response passing through the adapter raises:
NameError: uninitialized constant Protocol::HTTP1::Reason
async_http_client_adapter.rb:115 in `build_webmock_response'
Stubbed responses never reach that method, so a suite stays green until
something makes a real request. It surfaced for us via a library that
recently moved its HTTP transport to async-http: every pass-through
request raised, and the failure looked nothing like a WebMock problem.
Resolves the description table on first use rather than at load, so it
does not depend on which of the two libraries is required by then, and
keeps working either side of protocol-http1 0.40.0.
The spec helper's build_hash_response had the same reference and is
routed through the same method.
One existing example does reach the broken line, the cross-concern one
that records a real response from WebMockServer and plays it back: on
master it fails with the NameError, and in a full-file run the
connection pool then never drains and the run hangs. The added
regression test pins the status message itself, which that example does
not assert.
3a40e9b to
a50557e
Compare
|
Thanks! |
Fixes #1129
What
protocol-http10.40.0 removedProtocol::HTTP1::Reasonin favour ofProtocol::HTTP::Status, but the async-http adapter still readsProtocol::HTTP1::Reason::DESCRIPTIONSwhen building a WebMock response. Any real response passing through the adapter raises:spec/acceptance/async_http_client/async_http_client_spec_helper.rbcarried the same reference inbuild_hash_response; it now goes through the same method.Why it went unnoticed
Almost everything in the async-http suite is stubbed, and stubbed responses never reach
build_webmock_response.One existing example does reach it:
allows a response with multiple values for the same header to be recorded and played back exactly as-is, which records a real response fromWebMockServer. Without this change it fails with theNameErrorabove, and in a full-file run the connection pool then never drains, so the run hangs instead of reporting the failure. That example asserts header and body equality, so it would not catch a wrong-but-present status message. The new test pins that specifically.It surfaced for us via a library that recently moved its HTTP transport to async-http. Every pass-through request raised, and because the adapter sits underneath the HTTP client, the failure looked nothing like a WebMock problem.
Testing
Ruby 3.4.7,
async-http0.99.0,protocol-http10.40.2,NO_CONNECTION=1so only the local examples run.NameErroratasync_http_client_adapter.rb:115async_http_client_spec.rbrunWaiting for Async::HTTP::Protocol::HTTP pool to drainNameErroratasync_http_client_adapter.rb:115The single remaining failure,
should raise exception if request was made to different scheme, reproduces in isolation on a cleanmastercheckout, so it is pre-existing and untouched here.Out of scope
The
net_connect: trueexamples that reachhttpstat.usfail withEOFErrorraised from@network_client.callon this machine. They reproduce identically on an unmodifiedmaster, before and after this change.