Allow then-able Faraday responses for free client concurrency

[theojulienne]

👋 I'm working on creating improved support for non-threaded concurrent IO workloads at GitHub, and am creating this PR since we use twirp-ruby and it would be great to see it support one small change that would make it easy to enable concurrent requests on a single thread.

This PR adds support for executing the mutation from a Faraday response to ClientResp in a .then { ... } block when the object supports #then, as is common for promises. This allows extensions of Faraday such as iopromise-faraday to return a Promise of a response rather than an immediate response, and have twirp-ruby work exactly as expected.

This doesn't introduce any dependencies on promise libraries (or any other libraries) or change the behaviour in the normal case.

When a provided Faraday client (with extended behaviour) returns a response supporting .then { ... }, it is used. In Ruby 2.6+, all objects support .then { ... } but it behaves just like a Promise. In older Ruby (which the gemspec indicates support for), this code simply runs the function as before and so should be fully compatible.

The end result is a no-op change for folks using this library right now, but for those adopting IOPromise, this introduces free single-threaded concurrency of RPC calls using this gem - take the HelloWorld service from the example in this repo being slightly amended to be slow:

# Service implementation
class HelloWorldHandler
  def hello(req, env)
    sleep 1
    @count ||= 1
    @count += 1
    puts ">> Hello #{req.name} #{@count} #{Thread.current}"
    {message: "Hello #{req.name} #{@count}"}
  end
end

And the client, providing the Twirp client an IOPromise-supporting Faraday that returns a promise (responding to #then):

puts Time.now

require 'iopromise/faraday'

conn = IOPromise::Faraday.new('http://localhost:8080/twirp')
c = Example::HelloWorld::HelloWorldClient.new(conn)

ps = (0..10).map do
  c.hello(name: "World").then do |resp|
    if resp.error
      puts resp.error
    else
      puts resp.data.message
    end
  end
end

Promise.all(ps).sync

puts Time.now

This results in concurrent requests, rather than sequential ones:

2021-06-24 06:55:37 +0000
Hello World 37
Hello World 39
Hello World 37
Hello World 38
Hello World 43
Hello World 42
Hello World 41
Hello World 40
Hello World 46
Hello World 45
Hello World 44
2021-06-24 06:55:38 +0000

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[theojulienne]

👋 I just wanted to reach out to see if there was anything I could add here to make this PR easier to review/accept?

We're using this library quite heavily at GitHub and it would be great to be able to support concurrent RPC requests in the way described above, to make GitHub itself run faster 😀 🚀

Thanks in advance, and thank you for maintaining this library! 💖

[rudle]

Hi @theojulienne thanks for the contribution! We're excited that twirp-ruby is valuable to GitHub.

I'm currently getting up to speed with how the IO library you're developing works and will complete this review soon.

[rudle]

Hi @theojulienne, I thought about it a bit and I wonder if a more generic approach to extending #rpc would suffice for this use case while maintaining flexibility.

I've done a code sketch with a small test here https://github.com/twitchtv/twirp-ruby/compare/main...rudle:rpc-with-blocks?expand=1. The idea I'm exploring is what if the generated RPC instance methods accepted an optional block that yielded the response? In this world, client code that wanted to use any promise library could do so in whatever way was natural for them. I'm not sure this meets your goals, so please do take a look at my suggestion.

I have a weak preference to the block approach as long as it meets your needs because it's more on the happy path of the syntax of the language. While it's true that the #then keyword is a no-op in the non-promise library case it's not a popular way to extend behavior so it sticks out a bit.

[theojulienne]

I'm not sure this meets your goals, so please do take a look at my suggestion.

Thanks so much for looking into this PR/idea!

🤔 It would be really interesting to do this with blocks, though I'm not sure how to do it while still being asynchronous.

If I'm reading it right, the library code that converts a Faraday response to a Twirp ClientResp also needs to be deferred until after the request completes, which in the case of iopromise-faraday is after a promise resolves. In this patch, I think the ordering is:

• Call the RPC method, HTTP call is made
• Block is called with the Faraday HTTP response object (or a promise for one)
• After the block is executed, the same object (faraday response or promise) has .status, .headers etc called to generate a ClientResp, which won't work if the response is in progress
• ClientResp is returned from the original call

Because the last bit there after the HTTP call is executed synchronously in the same block, it's not possible to ever delay execution of the block or follow up code - essentially what we need is to somehow be deferring the execution of the block to some future point where the response is ready and then continue on with other tasks until then. Promises support this pattern by deferring the block until some other action resolves the promise, allowing multiple requests to be created and queued up but not have the work done until they complete. Additionally, we would want the ClientResp, not the Faraday response to be available to the application code.

It would be possible to pass a block into the #rpc function to operate on the response, but this would need to be somehow performed asynchronously, after the request has completed but with the mutated ClientResp object. This could be achieved with promises by passing the &block to a promise with #then, but at that point we'd be back to calling #then, but taking the application side code as an argument rather than using the promise .then {} pattern.

Is there any sort of pattern for deferring (past the point of returning from #rpc) the execution of the Faraday response -> ClientResp and follow on application logic that would feel like a more common pattern?

[rudle]

Hi @theojulienne, good point about the block approach, thanks for considering it.

I can't think of a more natural way of expressing this pattern, so this is probably the way to go while still keeping things backward compatible (all the way back to 1.9!).

I'm supportive of this change landing and will ask one of my colleagues with write access to merge it soon.

Thanks for the making a contribution to twirp-ruby!

[rudle]

One more small licensing requirement from our side. Please confirm that you understand the license of this project by including this phrase in the PR description.

"By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license."

[theojulienne]

Thanks @rudle!

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

(I put it in the PR description too)