Possible memory leak?

[ioquatix]

Imagine a vanilla rails app, where controller actions might look like:

class SomeController < ApplicationController
  def update
    response = request_handler.do_post("https://microsoft.com/update_thing", params[:post_body])
    render json: response.body
  end

  def request_handler
    @_request_handler ||= RequestHandler.new
  end
end

and RequestHandler something like:

class RequestHandler
  def do_post(url, body)
    task = Async do
      conn.post(url) do |req|
        req.body = body
      end
    end
    response = task.wait
  end

  def conn
    Faraday.new do |c|
      c.adapter :async_http
    end
  end
end

the app is using falcon host or falcon virtual with the rack configuration, a preload of the rails environment, and endpoint Async::HTTP::Endpoint.parse("http://0.0.0.0:3000"). pretty simple.

Originally posted by @jakeonfire in #32 (comment)

[ioquatix]

I believe you need to invoke #close on conn after you are done with it... Are you able to check if this solves your problem? I will also take a look on my end if I have time.

[jakeonfire]

i thought about that but it doesn't seem closeable. faraday should close the adapter automatically after every request afaik.

NoMethodError: undefined method `close' for #<Faraday::Connection...

[ioquatix]

It should be this one:

https://github.com/lostisland/faraday/blob/main/lib/faraday/connection.rb#L122-L127

[jakeonfire]

ah i'm looking at the wrong source. let me try with faraday 1.x.

[jakeonfire]

#close works. canarying the change in production...

would it make sense to restrict async-http-faraday to faraday >= 1.0? might help others to put a call to close in the README instructions, as well. something like:

task = Async do
	response = conn.get("/index")
ensure
	conn.close
end

[ioquatix]

I recommend trying the

begin
  connection = ...
  connection.get ...
ensure
  connection.close
end

model to start with (as it seems you are). However, you won't get persistent connections this way.

So, after you tried that, use the thread-local gem: https://github.com/socketry/thread-local?tab=readme-ov-file to share an instance:

connection = MyConnection.instance
connection.get ...

Using the persistent connection will improve throughput and latency.

[jakeonfire]

getting a different issue now

[jakeonfire]

despite the new issue, memory usage is stable, so closing the connection seems to have been the ticket 🥳

[jakeonfire]

Faraday's myriad instantiation-time configurations and state makes sharing a connection difficult. what if different requests from the same thread need different middleware?

i guess i could create a separate thread-local class for each configuration, or a single thread-local with a hash of different options => different faraday connections...

[ioquatix]

Yeah, I understand your issue. For the sake of persistent connections, maybe we want a shared internal pool of HTTP connections. Most of the request/response processing won't actually impact the underlying connection pool...

[jakeonfire]

yeah that sounds about right. i may take a look.

[jakeonfire]

you can close this issue 🙂

[jakeonfire]

Using the persistent connection will improve throughput and latency.

@ioquatix do you think using a process-wide connection pool instead of a thread-local one, via https://github.com/mperham/connection_pool (or https://github.com/socketry/async-pool which i think does the same thing?), might be more efficient in some ways?

i noticed async-http uses its own connection pool: https://github.com/socketry/async-http/blob/main/lib/async/http/client.rb#L39

[ioquatix]

No, you should avoid doing additional connection pooling.

The reason is, there are life-cycle issues at play.

Using thread-local for your connection object is the best but... I'll try to default to the shared connection pool provided by async-http. Please give me a day to check it and come back to you.