Fix leaky file descriptors and reuse socket for persistent connections #976

rzane · 2022-05-24T04:03:20Z

Take the following example:

require 'net/http'
require 'webmock'
WebMock.enable!
WebMock.allow_net_connect!

http = Net::HTTP.new('example.org')
http.start
http.get('/')
http.get('/')
http.get('/')
http.finish

This example would create 3 sockets and only close one of them. That's because start_with_connect_without_finish spawns a new connection for each request, and never closes the previous one.

This PR also makes it so that when using a persistent connection, the @socket will be reused across requests. I believe this should totally fix the common "gotcha" that people encounter when combining WebMock with Capybara without needing to enable net_http_connect_on_start.

I'd recommend reviewing this PR commit-by-commit. I tried to provide pretty detailed explanations for the changes as I made them.

When explicitly calling `Net::HTTP#start`, a stub socket should be initialized.

…ry here

When using a persistent connection, new sockets will be created for every single request and they are never closed. This commit makes it so that the current socket is closed before a new one is connected.

When using a persistent connection, the point is to reuse the underlying socket. Prior to this commit, the socket would be thrown away before each request unless `net_http_connect_on_start` is `true`. This commit will preserve the previously started socket if the socket represents an actual connection.

kbruccoleri · 2022-06-10T17:54:39Z

👍 I have also been experiencing this issue, this seemed to do the trick for me.

bblimke · 2022-08-02T10:06:14Z

Hi @rzane. Thank you for the PR with very descriptive commit messages. It's way cleaner and understandable now. 👍

jdufresne · 2022-08-03T03:46:18Z

Hi, This change resulted in a new error for my test suite. I tracked it down to commit 18dc8a9.

The error appears as shown below. At first glance it looks like the new stub objects are missing some methods.

 NoMethodError:
   undefined method `ssl_version' for #<StubSocket::StubIO:0x00007f08a997b388>

         { version: @socket.io.ssl_version, cipher: @socket.io.cipher[0] }
                              ^^^^^^^^^^^^
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/net_http_ssl_connection.rb:8:in `ssl_connection'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/connection.rb:82:in `block (2 levels) in request'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/connection.rb:80:in `block in request'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/network_connection_retries.rb:24:in `block in retry_exceptions'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/network_connection_retries.rb:52:in `retry_network_exceptions'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/network_connection_retries.rb:23:in `retry_exceptions'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/connection.rb:75:in `request'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/posts_data.rb:74:in `raw_ssl_request'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/posts_data.rb:44:in `ssl_request'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/posts_data.rb:40:in `ssl_post'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/billing/gateways/authorize_net.rb:817:in `commit'
 # ./.bundle/ruby/3.1.0/gems/activemerchant-1.126.0/lib/active_merchant/billing/gateways/authorize_net.rb:120:in `authorize'

jdufresne · 2022-08-03T04:19:21Z

lib/webmock/http_lib_adapters/net_http.rb

@@ -119,26 +114,21 @@ def start_without_connect
          raise IOError, 'HTTP session already opened' if @started
          if block_given?
            begin
+              @socket = Net::HTTP.socket_type.new


FWIW, Net::HTTP.socket_type is obsolete:

https://github.com/ruby/net-http/blob/196f3d70f9ce154dc63a86a2cdbf1aec4df4982e/lib/net/http.rb#L575-L577

bblimke · 2022-08-03T09:08:40Z

@jdufresne thank you for reporting.

I wonder if this is related to Ruby 3.1 or happens in earlier versions as well.

I guess it's not easy for you to provide a code sample or spec to reproduce it, since it's active merchant?

Am I right that it happens when the request is stubbed and not for the real request?

Perhaps StubIO should have more methods implemented, but then why has it not been happening before. Perhaps StubIO was not used.

@rzane any ideas?

jdufresne · 2022-08-03T13:44:13Z

Am I right that it happens when the request is stubbed and not for the real request?

That is right. This only occurs in the test suite when the request is stubbed.

Perhaps StubIO should have more methods implemented, but then why has it not been happening before. Perhaps StubIO was not used.

IIUC, StubIO wasn't introduced until 18dc8a9 which is why it was not used before. This has only been included in the most recent release. Downgrading fixes the issue for me.

Adding the two methods ssl_version and cipher fixes the issue for me, but:

Do other users rely on another missing method?
Is just adding blank methods the way we want to go here?
Does ssl_version apply to both HTTP and HTTPS connection or just HTTPS?

bblimke · 2022-08-03T14:05:33Z

StubIO was introduced way ago c4b232b but obviously now it's being set in more scenarios than before and I wonder what are these scenarios, where despite stubbed request, normal socket was used instead of stubbed one with StubIO. I would have thought that StubIO was always used for stubbed requests, but obviously not.

rzane · 2022-08-03T14:59:07Z

The issue @jdufresne is seeing is coming from the fact that ActiveMerchant are getting information from the @socket, which is a private instance variable. See here.

Previously, they actually had to work around the fact that WebMock was not assigning @socket (and therefore not behaving like Net::HTTP) in the past: activemerchant/active_merchant@79c46f7.

I do think WebMock should act like Net::HTTP as much as possible. Therefore, I think the solution here is to add more methods to the StubIO, which as you said @bblimke, has been around forever.

For HTTP connections, @socket.io should act like a TCPSocket.
For HTTPS connections, @socket.io should act like a OpenSSL::SSL::SSLSocket.

jdufresne · 2022-08-04T01:19:01Z

Here is a minified test case that can be used to dig deeper. The following script works with 3.15 but not 3.16.

require 'activemerchant'
require 'webmock'
include WebMock::API

WebMock.enable!

stub_request(:post, "https://api2.authorize.net/xml/v1/request.api")

ActiveMerchant::Billing::Base.mode = 'test'

credit_card = ActiveMerchant::Billing::CreditCard.new({
  number: '4111111111111111',
  month: '11',
  year: '25',
  first_name: 'John',
  last_name: 'Doe',
  verification_value: '111',
})

gateway = ActiveMerchant::Billing::AuthorizeNetGateway.new(
  login: 'test',
  password: 'test',
)

gateway.authorize(1000, credit_card)

bblimke · 2022-08-04T09:26:54Z

@rzane thank you for the investigation and explanation.

@jdufresne thank you for the code sample.

Extending StubIO to the following form works:

class StubIO
    def setsockopt(*args); end
    def peer_cert; end
    def ssl_version; end
    def cipher; []; end
  end

I'm not sure this is the right approach though. According to OpenSSL docs,
cipher should either return nil or an array of size 4.

I think the right approach should be returning nil, since there is no cipher,
yet activemerchant compatibility patch activemerchant/active_merchant@79c46f7 expects cipher to be an Array in case the socket it present.
Perhaps StubIO#cipher should return nil and activemerchant should make the change to not
expect cipher to be an array, but allow it to be nil.

rzane added 4 commits May 23, 2022 20:48

Calling Net::HTTP#start should assign a socket

18dc8a9

When explicitly calling `Net::HTTP#start`, a stub socket should be initialized.

The block version of start_with_connect_without_finish is unnecessa…

7a2a406

…ry here

Close the socket before spawning a new one

3d3bdc2

When using a persistent connection, new sockets will be created for every single request and they are never closed. This commit makes it so that the current socket is closed before a new one is connected.

slayer approved these changes Jun 22, 2022

View reviewed changes

bblimke merged commit dc6ff71 into bblimke:master Aug 2, 2022

jdufresne reviewed Aug 3, 2022

View reviewed changes

jdufresne mentioned this pull request Aug 4, 2022

Incompatible with WebMock 3.16.0 activemerchant/active_merchant#4523

Closed

jibees mentioned this pull request Aug 4, 2022

Bump webmock from 3.15.0 to 3.16.0 openfoodfoundation/openfoodnetwork#9508

Closed

rzane mentioned this pull request Aug 5, 2022

Make the StubSocket slightly more believable #988

Merged

bobmaerten mentioned this pull request Aug 22, 2022

Probable incompatibility with Selenium? #1001

Closed

rzane deleted the close-sockets branch March 16, 2023 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix leaky file descriptors and reuse socket for persistent connections #976

Fix leaky file descriptors and reuse socket for persistent connections #976

rzane commented May 24, 2022 •

edited

kbruccoleri commented Jun 10, 2022

bblimke commented Aug 2, 2022

jdufresne commented Aug 3, 2022 •

edited

jdufresne Aug 3, 2022

bblimke commented Aug 3, 2022

jdufresne commented Aug 3, 2022

bblimke commented Aug 3, 2022

rzane commented Aug 3, 2022

jdufresne commented Aug 4, 2022

bblimke commented Aug 4, 2022

Fix leaky file descriptors and reuse socket for persistent connections #976

Fix leaky file descriptors and reuse socket for persistent connections #976

Conversation

rzane commented May 24, 2022 • edited

kbruccoleri commented Jun 10, 2022

bblimke commented Aug 2, 2022

jdufresne commented Aug 3, 2022 • edited

jdufresne Aug 3, 2022

Choose a reason for hiding this comment

bblimke commented Aug 3, 2022

jdufresne commented Aug 3, 2022

bblimke commented Aug 3, 2022

rzane commented Aug 3, 2022

jdufresne commented Aug 4, 2022

bblimke commented Aug 4, 2022

rzane commented May 24, 2022 •

edited

jdufresne commented Aug 3, 2022 •

edited