Hangs after warning: "Waiting for pool to drain" from Async::HTTP::Client

[joekhoobyar]

Clearly, I am doing something wrong.

I have two successful GET requests, via an Async::REST::Representation, then my following PATCH request gives the following warning - and then the script hangs forever:

0.14s     warn: Async::HTTP::Client [oid=0x2b1f879ac44c] [pid=22880] [2020-03-01 03:15:27 +0000]
               | Waiting for pool to drain: #<Async::Pool::Controller(1/∞) 1/1*/1>

If it will be useful, I can post my code. It does not have any explicit reactor usage, it's just letting the framework take care of instantiating the reactor every time (which I know is not very performant, but it's just a test script for now).

[joekhoobyar]

# test.rb
decorator = EDMS::MayanDecorator.new 1
$stdout.puts decorator.metadata.inspect
decorator.send :write_document_metadata, 12, :vendor_name, 'Shanks'

# edms/decorate.rb
require 'base64'

require 'async/http/endpoint'
require 'async/rest/representation'

module EDMS
  module Mayan
    class Representation < Async::REST::Representation
    end

    class Client < Representation
      def document(id)
        with Document, path: "documents/#{id}/"
      end

      def document_type(id)
        with DocumentType, path: "document_types/#{id}/"
      end
    end

    class Document < Representation
      def metadata(id=nil)
        if id.nil?
          with DocumentMetadatas, path: "metadata/"
        else
          with DocumentMetadata, path: "metadata/#{id}/"
        end
      end
    end

    class DocumentMetadata < Representation
    end

    class DocumentMetadatas < Representation
      def results
        value[:results]
      end
    end

    class DocumentType < Representation
      def metadata_types
        with DocumentTypeMetadataTypes, path: "metadata_types/"
      end
    end

    class DocumentTypeMetadataTypes < Representation
      def results
        value[:results]
      end
    end
  end

  class MayanDecorator
    attr_reader :document_type_id, :connection

    DEFAULT_CONNECTION = { 
      url: ENV['MAYAN_EDMS_URL'],
      user: ENV['MAYAN_EDMS_USER'],
      password: ENV['MAYAN_EDMS_PASSWORD']
    }

    def initialize(document_type_id, connection: DEFAULT_CONNECTION)
      @document_type_id = document_type_id
      @connection = connection
    end

    def metadata
      @metadata ||= metadata!
    end

    protected

    def metadata!
      with_client do |client|
        types = client.document_type(document_type_id).metadata_types
        Hash[ types.results.map { |r| r[:metadata_type].values_at(:name, :id) } ]
      end
    end

    def write_document_metadata(document_id, name, value)
      metadata_id = metadata[name.to_s]
      raise ArgumentError, "no such metadata named '#{name}'" if metadata_id.nil?

      with_client do |client|
        client.document(document_id).metadata(metadata_id).patch('value' => value)
      end
    end

    private

    def with_client
      result = nil
      task = Mayan::Client.for "#{connection[:url]}/api/", client_headers do |client|
        result = yield client
      end
      result
    end

    def client_headers
      headers = Protocol::HTTP::Headers.new
      userpass = "#{connection[:user]}:#{connection[:password]}"
      headers['Authorization'] = "Basic #{Base64.encode64(userpass).chomp}"
      headers
    end
  end
end

[joekhoobyar]

It's clear from the stacktrace that is failing to set up the reactor:

 0.19s     warn: Async::HTTP::Client [oid=0x2b10eb8cd404] [pid=28626] [2020-03-01 03:25:19 +0000]
               | Waiting for pool to drain: #<Async::Pool::Controller(1/∞) 1/1*/1>
^C/home/joe/.gem/ruby/2.4.0/gems/async-1.24.2/lib/async/reactor.rb:223:in `run_once': Interrupt
	from /home/joe/.gem/ruby/2.4.0/gems/async-1.24.2/lib/async/reactor.rb:234:in `run'
	from /home/joe/.gem/ruby/2.4.0/gems/async-1.24.2/lib/async/reactor.rb:56:in `run'
	from /home/joe/.gem/ruby/2.4.0/gems/async-1.24.2/lib/kernel/async.rb:28:in `Async'
	from /home/joe/.gem/ruby/2.4.0/gems/async-rest-0.12.2/lib/async/rest/representation.rb:46:in `for'
	from /home/joe/src/edms/edms-analyzer/lib/edms/decorate.rb:93:in `with_client'
	from /home/joe/src/edms/edms-analyzer/lib/edms/decorate.rb:84:in `write_document_metadata'
	from test.rb:16:in `<main>'

[joekhoobyar]

Appending a .read following the call to patch('foo' => 'bar') fixes the error.

This is not very intuitive at all, but I'm not sure how I could expect a library centered around asynchronous I/O to perform the read "for me".

I am torn here.

• Should we fix this in the documentation or in the examples?
• Should we automatically "drain" an assumed discarded response body on subsequent HTTP requests?

[ioquatix]

You can call finish or close on the response object. Close will immediately terminate the connection and discard any response body. If there is no response body the connection will return to the connection pool. Finish will gracefully finish reading the response and return the connection to the pool.

[ioquatix]

The last reply was from my phone. Here are a few more thoughts...

So, basically, when you make request, the response is provided as soon as the server responds with a complete set of headers.

This is a bit different from other libraries that will fully read/buffer the response data too.

In the past, these operations were a little bit different: https://github.com/socketry/protocol-http/blob/master/lib/protocol/http/body/reader.rb#L48-L57

The logic to these methods right now, is that once you read the body, it's finished.

Previously, #finish was implemented like:

@body = @body.finish

This allowed you to cache the body in the response... but once you call #read it would clear out the body, so it makes the response stateful in a very confusing way. Unless the logic was #read more than once is okay if the response is buffered... it turns into a bit of a rabbit hole. So now the methods are one-shot.

So, the logic of the response is you must do one of those things: https://github.com/socketry/protocol-http/blob/master/lib/protocol/http/body/reader.rb#L27-L76

I'm open to ideas about how to do this better. However, maybe there is something wrong with async-rest too - i.e. maybe there should be slightly higher methods. That being said, it should be in your hands how to deal with methods like PATCH.

So, async-rest follows the REST thesis which a representation is loaded from a remote resource, and this naturally reads the response body, e.g.

async-rest/lib/async/rest/representation.rb

Lines 114 to 123 in a3ba050

	def value!
	response = self.get
	if response.success?
	@metadata = response.headers
	@value = response.read
	else
	raise ResponseError, response
	end
	end

Looking at your logic:

client.document(document_id).metadata(metadata_id).patch('value' => value)

The biggest issue with this is you are ignoring the response status. It might fail, which should raise an error.

What is the response of patch, does it update the state of the object?

If there is some kind of standard model, I'd be willing to consider figuring out the semantic model that works better.

For example, my preference is something like this:

    class DocumentMetadata < Representation
      def update(key, value)
        response = self.patch(key => value)
        if response.success?
          @value = response.read # or something similar
        else
          raise SomeError, response.read
        end
      end
    end

Anyway, the logic of reading responses should probably follow the same path through the wrapper... unless there is some specific reason not to.

[joekhoobyar]

Even for a POC, yes: ignoring the response status is bad. 👍

I am completely new to the methodologies needed for async REST API calls - so this is good stuff.

I have worked through the issues on my side by making sure to always close my response after dealing with it.

I like the idea you have of making sure these calls go through the wrapper.

[joekhoobyar]

Side question, @ioquatix, can you point me to any basic resources for the following:

• Good strategies for dealing with the async reactor during RSpec tests? So far everything "Just Works", but I'd like to know if there's anything I should look out for.
• Examples or good strategies of mocking / stubbing for a Resource or Represention in Rspec?
• Ability to enable logging of requests made async-http:
  • For local debugging
  • For general log output, to trace work that was done
  • Possibly even the ability to inspect payloads in the log

[ioquatix]

Good strategies for dealing with the async reactor during RSpec tests? So far everything "Just Works", but I'd like to know if there's anything I should look out for.

Make sure you use Async::RSpec: https://github.com/socketry/async-rspec

It enables debug assertions and other checks.

Examples or good strategies of mocking / stubbing for a Resource or Represention in Rspec?

There is support for this in webmock and we are also building a native wrapper to enable mocked requests/responses, which should be a little bit better: socketry/async-http#41

Ability to enable logging of requests made async-http:

This could probably be improved. Logging payloads is a bit more tricky.

CONSOLE_DEBUG=Async::HTTP::Client rspec

will enable logging from instances of Async::HTTP::Client... maybe we can improve this.

[joekhoobyar]

Awesome, that gives me enough to get going for now! Thank you.

[ioquatix]

I am going to close this issue as it appears your problems are resolved.