-
Notifications
You must be signed in to change notification settings - Fork 556
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
Simplify Net::HTTP connection handing #992
Changes from all commits
7f7eba8
28e3c9a
d327547
5567e1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,78 +72,57 @@ def constants(inherit=true) | |
end | ||
|
||
def request(request, body = nil, &block) | ||
return super unless started? | ||
|
||
request_signature = WebMock::NetHTTPUtility.request_signature_from_request(self, request, body) | ||
|
||
WebMock::RequestRegistry.instance.requested_signatures.put(request_signature) | ||
|
||
if webmock_response = WebMock::StubRegistry.instance.response_for_request(request_signature) | ||
@socket = Net::HTTP.socket_type.new | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no reason to replace the socket here. At the very least, we'd want to close the old socket before initializing a new one. This would have caused a file descriptor leak in the scenario where the first request was allowed, but the second was stubbed. |
||
WebMock::CallbackRegistry.invoke_callbacks( | ||
{lib: :net_http}, request_signature, webmock_response) | ||
build_net_http_response(webmock_response, &block) | ||
elsif WebMock.net_connect_allowed?(request_signature.uri) | ||
check_right_http_connection | ||
after_request = lambda do |response| | ||
if WebMock::CallbackRegistry.any_callbacks? | ||
webmock_response = build_webmock_response(response) | ||
WebMock::CallbackRegistry.invoke_callbacks( | ||
{lib: :net_http, real_request: true}, request_signature, webmock_response) | ||
end | ||
response.extend Net::WebMockHTTPResponse | ||
block.call response if block | ||
response | ||
end | ||
super_with_after_request = lambda { | ||
response = super(request, nil, &nil) | ||
after_request.call(response) | ||
} | ||
if started? | ||
ensure_actual_connection | ||
super_with_after_request.call | ||
else | ||
start_with_connect { | ||
super_with_after_request.call | ||
} | ||
ensure_actually_connected | ||
|
||
response = super(request, nil, &nil) | ||
|
||
if WebMock::CallbackRegistry.any_callbacks? | ||
WebMock::CallbackRegistry.invoke_callbacks( | ||
{lib: :net_http, real_request: true}, | ||
request_signature, | ||
build_webmock_response(response) | ||
) | ||
end | ||
|
||
response.extend Net::WebMockHTTPResponse | ||
block.call response if block | ||
response | ||
else | ||
raise WebMock::NetConnectNotAllowedError.new(request_signature) | ||
end | ||
end | ||
|
||
def start_without_connect | ||
raise IOError, 'HTTP session already opened' if @started | ||
if block_given? | ||
begin | ||
@socket = Net::HTTP.socket_type.new | ||
@started = true | ||
return yield(self) | ||
ensure | ||
do_finish | ||
end | ||
end | ||
@socket = Net::HTTP.socket_type.new | ||
@started = true | ||
self | ||
end | ||
private | ||
|
||
alias_method :actually_connect, :connect | ||
|
||
def ensure_actual_connection | ||
if @socket.is_a?(StubSocket) | ||
@socket&.close | ||
@socket = nil | ||
do_start | ||
end | ||
end | ||
|
||
alias_method :start_with_connect, :start | ||
|
||
def start(&block) | ||
def connect | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
uri = Addressable::URI.parse(WebMock::NetHTTPUtility.get_uri(self)) | ||
|
||
if WebMock.net_http_connect_on_start?(uri) | ||
super(&block) | ||
super | ||
else | ||
start_without_connect(&block) | ||
@socket = StubSocket.new | ||
end | ||
end | ||
|
||
def ensure_actually_connected | ||
if @socket.is_a?(StubSocket) | ||
@socket&.close | ||
@socket = nil | ||
actually_connect | ||
end | ||
end | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -215,14 +215,14 @@ class TestMarshalingInWebMockNetHTTP | |
|
||
if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('2.7.0') | ||
it "uses the StubSocket to provide IP address" do | ||
Net::HTTP.start("http://example.com") do |http| | ||
Net::HTTP.start("example.com") do |http| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is me fixing a mistake I made in #988 😄 |
||
expect(http.ipaddr).to eq("127.0.0.1") | ||
end | ||
end | ||
end | ||
|
||
it "defines common socket methods" do | ||
Net::HTTP.start("http://example.com") do |http| | ||
Net::HTTP.start("example.com") do |http| | ||
socket = http.instance_variable_get(:@socket) | ||
expect(socket.io.ssl_version).to eq("TLSv1.3") | ||
expect(socket.io.cipher).to eq(["TLS_AES_128_GCM_SHA256", "TLSv1.3", 128, 128]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lets us avoid having to deal with the case where the connection is not started yet. It makes the implementation much simpler.
see https://github.com/ruby/net-http/blob/196f3d70f9ce154dc63a86a2cdbf1aec4df4982e/lib/net/http.rb#L1568-L1574
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever! 👍