Avoiding closuring a Proc that's an instance variable

[jgaskins]

I’ve got an API client object that receives a block to generate an auth token. I can’t use static auth tokens because the API uses JWTs with expiration timestamps, so I need to generate them at request time.

The initializer looks like this:

class Client
  def initialize(@base_uri = default_uri, &@token_generator : -> String)
    @pool = DB::Pool(HTTP::Client).new(DB::Pool::Options.new(initial_pool_size: 0)) do
      http = HTTP::Client.new(base_uri)
      http.before_request do |request|
        request.headers["Authorization"] = "Bearer #{token_generator.call}"
        request.headers["Accept"] = "application/json"
        request.headers["Connection"] = "keep-alive"
      end
      http
    end
  end
end

I want to be able to pass that connection pool to a different instance so it can reuse the TCP sockets if it needs to use an OAuth2 access token for a user. What I’m currently attempting duplicates the client, sets the token generator, and returns that instance:

def with_access_token(&token_generator : -> String)
  new = dup
  new.token_generator = token_generator
  new
end

However, since I’m passing token_generator to DB::Pool in the initializer, setting a new one on the dup doesn’t do anything because I’ve closured the original in the DB::Pool block. Any ideas?

[Blacksmoke16]

A few thoughts come to mind:

1. Can you defer adding of the authorization header to actual request time outside of before_request? This way you wouldn’t have to closure the token generator at all, at the cost of not being able to leverage before_request.
2. Could you call the token generator outside of the block and assign the result to a variable and use that within the block? Depending on how long the token expiry is and how long lived the client is, might expire before its used tho.
3. Could you move how the client is instantiated to some other class? This would allow having the block closure an instance of like HTTPClientFactory instead, which if you keep a reference to could override token_generator on it before its used to create a client.

[jgaskins]

@Blacksmoke16

Can you defer adding of the authorization header to actual request time outside of before_request? This way you wouldn’t have to closure the token generator at all, at the cost of not being able to leverage before_request.

This is a pretty solid idea. I started using before_request in all my API clients because it’s easier than manually passing headers each time like I used to do — every method that called HTTP::Client methods would basically call it with headers: DEFAULT_HEADERS.dup.merge!(headers). I guess I’ve been doing that for so long that my mind was like “ha ha you can forget the other thing now because this is better”.

@Blacksmoke16

Could you call the token generator outside of the block and assign the result to a variable and use that within the block? Depending on how long the token expiry is and how long lived the client is, might expire before its used tho.

I was thinking about this, too, but I couldn’t come up with a way to make it work.

@Blacksmoke16

Could you move how the client is instantiated to some other class? This would allow having the block closure an instance of like HTTPClientFactory instead, which if you keep a reference to could override token_generator on it before its used to create a client.

Since the pool is what I want to share, and the HTTP::Client instances are nested inside that, I didn’t have a good way to make that work. However, this sparks an idea.

Since what I’m actually using is a subclass of HTTP::Client, I could maybe setup some wiring there:

     def get(path : String, as type : T.class) : T forall T
-      response = @pool.checkout &.get(path)
+      response = @pool.checkout do |http|
+        http.authorizer = self
+        http.get(path)
+       end

       # ...
     end

I think your #1 is the safest bet. It adds duplication but it’s clear. This implementation of #3 adds cleverness (which is fun to sow but less fun to reap) and I’d have to run it on each method anyway, so it’s not any less duplication than #1.

[Blacksmoke16]

@jgaskins

Since the pool is what I want to share, and the HTTP::Client instances are nested inside that, I didn’t have a good way to make that work

I think it would just be like:

class Client
  def initialize(@base_uri = default_uri, @http_client_factory)
    @pool = DB::Pool(HTTP::Client).new(DB::Pool::Options.new(initial_pool_size: 0)) do
      @http_client_factory.create_client
    end
  end
end

The pool would closure the client factory ivar class, the token_generator would be set on that class, which means if its updated, the closured instance would get the updates too so future calls to #create_client would be using the latest generator.

@jgaskins

every method that called HTTP::Client methods would basically call it with headers: DEFAULT_HEADERS.dup.merge!(headers)

Might not be terrible to have something like private def send(request : HTTP::Request) : HTTP::Client::Response. Then there’s only 1 place you interact with the client via #exec. Also there’s no need to do any merging as you can still use before_request to set the static headers, then’d you just have to pass in like HTTP::Headers{"authorization" => @token_generator.call}, and other headers will be merged in for you.

[jgaskins]

@Blacksmoke16

I think it would just be like:

class Client
  def initialize(@base_uri = default_uri, @http_client_factory)
    @pool = DB::Pool(HTTP::Client).new(DB::Pool::Options.new(initial_pool_size: 0)) do
      @http_client_factory.create_client
    end
  end
end

The pool would closure the client factory ivar class, the token_generator would be set on that class, which means if its updated, the closured instance would get the updates too so future calls to #create_client would be using the latest generator.

I think this removes one layer of closures, but self in the DB::Pool block refers to the original Client instance (the one whose initialize method was called) when the object is duped, so a change to @http_client_factory in the second instance wouldn’t change @http_client_factory in the block. This is why @token_generator.call didn’t work in my original example.

@Blacksmoke16

Might not be terrible to have something like private def send(request : HTTP::Request) : HTTP::Client::Response. Then there’s only 1 place you interact with the client via #exec.

I love the way HTTP::Client does this, but I didn’t want to duplicate all that machinery. It’s one of those things that really makes me wish HTTP::Client supported connection pooling intrinsically. IIRC, there’s an issue open for that, right? I might add this use case to the discussion.

[Blacksmoke16]

@jgaskins

so a change to @http_client_factory in the second instance wouldn’t change @http_client_factory in the block.

I think at this point some example code would be helpful . Something like this?

record Bar, token : String

class BarFactory
  setter token_generator

  def initialize(&@token_generator : -> String); end

  def get_bar
    Bar.new @token_generator.call
  end
end

class Client
  @some_proc : Proc(Bar)

  def initialize(@bar_factory : BarFactory)
    # Some proc just to create a closure
    @some_proc = Proc(Bar).new do
      @bar_factory.get_bar
    end
  end

  def request : String
    @some_proc.call.token
  end

  def with_token(&token_generator : -> String) : self
    instance = self.dup
    instance.token_generator = token_generator
    instance
  end

  protected def token_generator=(token_generator : Proc(String)) : Nil
    @bar_factory.token_generator = token_generator
  end
end

bar_factory = BarFactory.new { "TOKEN_A" }

client = Client.new bar_factory

pp client.request # => "TOKEN_A"

new_token_client = client.with_token { "TOKEN_B" }

pp new_token_client.request # => "TOKEN_B"
pp client.request           # => "TOKEN_B"

Or are you expecting that client keeps using TOKEN_A while only new_token_client uses TOKEN_B?

@jgaskins

IIRC, there’s an issue open for that, right? I might add this use case to the discussion.

That would be Redesign HTTP::Client to implement missing features · Issue #6011 · crystal-lang/crystal · GitHub yea.

[jgaskins]

@Blacksmoke16

Or are you expecting that client keeps using TOKEN_A while only new_token_client uses TOKEN_B?

Yes, exactly this :-) When I’m making a request on behalf of the user, I create the duplicate client which generates a token for a specific user, but the main client should still generate tokens for the OAuth client itself, rather than a specific user.

For example, with the GitHub API:

github = GitHub::Client.new(client_id, client_secret)

I’ll need to use this client when talking to the GitHub API on behalf of my app itself. When I need to make a request for PRs, I need a client that makes requests on behalf of a specific user for the sake of things like permissions and quotas:

user_client = github.user_client { token_for(user, github, refresh: :if_expired) }

pr = user_client
  .repo(org_name, repo)
  .pull_requests
  .get(pr_number)

check_runs = user_client
  .repo(org_name, repo)
  .check_runs(ref: pr.head.ref)

We still need the github client to make requests on behalf of the app after calling user_client. I just want user_client to be able to also use github’s connection pool.

[Blacksmoke16]

, yea in that case not sure what your options would be. Kinda just spit-balling, but wonder if the pool could be made to accept some sort of arguments when creating a resource… Something like this:

diff --git a/src/db/pool.cr b/src/db/pool.cr
index 378e303..8edc8dc 100644
--- a/src/db/pool.cr
+++ b/src/db/pool.cr
@@ -3,7 +3,7 @@ require "weak_ref"
 require "./error"
 
 module DB
-  class Pool(T)
+  class Pool(T, *Args)
     record Options,
       # initial number of connections in the pool
       initial_pool_size : Int32 = 1,
@@ -62,16 +62,17 @@ module DB
 
     @[Deprecated("Use `#new` with DB::Pool::Options instead")]
     def initialize(initial_pool_size = 1, max_pool_size = 0, max_idle_pool_size = 1, checkout_timeout = 5.0,
-                   retry_attempts = 1, retry_delay = 0.2, &factory : -> T)
+                   retry_attempts = 1, retry_delay = 0.2, *args : *Args, &factory : Proc(*Args, T))
       initialize(
         Options.new(
           initial_pool_size: initial_pool_size, max_pool_size: max_pool_size,
           max_idle_pool_size: max_idle_pool_size, checkout_timeout: checkout_timeout,
           retry_attempts: retry_attempts, retry_delay: retry_delay),
+        *args,
         &factory)
     end
 
-    def initialize(pool_options : Options = Options.new, &@factory : -> T)
+    def initialize(pool_options : Options = Options.new, *args : *Args, &@factory : Proc(*Args, T))
       @initial_pool_size = pool_options.initial_pool_size
       @max_pool_size = pool_options.max_pool_size
       @max_idle_pool_size = pool_options.max_idle_pool_size
@@ -83,7 +84,7 @@ module DB
       @inflight = 0
       @mutex = Mutex.new
 
-      @initial_pool_size.times { build_resource }
+      @initial_pool_size.times { build_resource *args }
     end
 
     # close all resources in the pool
@@ -109,7 +110,7 @@ module DB
       )
     end
 
-    def checkout : T
+    def checkout(*args : *Args) : T
       res = sync do
         resource = nil
 
@@ -118,7 +119,7 @@ module DB
                        if can_increase_pool?
                          @inflight += 1
                          begin
-                           r = unsync { build_resource }
+                           r = unsync { build_resource *args }
                          ensure
                            @inflight -= 1
                          end
@@ -148,8 +149,8 @@ module DB
       res
     end
 
-    def checkout(&block : T ->)
-      connection = checkout
+    def checkout(*args : *Args, &block : T ->)
+      connection = checkout *args
 
       begin
         yield connection
@@ -236,8 +237,8 @@ module DB
       @idle.delete(resource)
     end
 
-    private def build_resource : T
-      resource = @factory.call
+    private def build_resource(*args : *Args) : T
+      resource = @factory.call *args
       sync do
         @total << resource
         @idle << resource

From here you can do something like:

@pool = DB::Pool(HTTP::Client, String).new(DB::Pool::Options.new(initial_pool_size: 0), @token_generator.call) do |token|
  http = HTTP::Client.new(base_uri)
  http.before_request do |request|
    request.headers["Authorization"] = "Bearer #{token}"
    request.headers["Accept"] = "application/json"
    request.headers["Connection"] = "keep-alive"
  end
  http
end

Which says the pool accepts a String block argument, passing the token to .new so its available if you wanted some clients created immediately. Otherwise you then do like:

@pool.checkout @token_generator.call do |client|
  # Do something ...
end

Wouldn’t really be a way to define defaults or anything. Nor would it help if the clients inside the pool aren’t built fresh all the time but . So yea, prob easiest to just do option 1 and call it a day .