[Proposal]: Map responses back to requests

[jch]

I'd like to propose tracking requests and responses by message id to support interleaved responses on the shared socket in Net::LDAP::Connection.

The Problem

Searches (and other operations) done on one instance of Net::LDAP::Connection share the same TCPSocket (code). This allows the caller to initiate requests before responses for prior requests have completed. Net::LDAP uses a shared variable @open_connection to memoize existing connections before executing an operation.

We run into trouble because we do not inspect the response message id's and try to map them back to their original requests. Consider the following:

# search1: This will instantiate an instance of `Net::LDAP::Connection`  and store it in `@open_connection`.
Net::LDAP.search(...) do |outer_entry|
   # search2: sub-search based on some attributes of entry
  Net::LDAP.search(:filter => ...) do |inner_entry|
  end
end

Because we don't map the message id back to the original search request, results can be interleaved:

client: search1         search2
-------------------------------------------------------------------------> time
server:         1  1  1          1  1  1  2  2  2  2  1  1  1
                                 ^  ^  ^
                                 |  |  |
                                 \  |  /
                                  \ | /
                   server is still responding to search1,
                   but search2 may see search1 results
                   because we're on the same socket

The Proposal

I created a proof of concept using fibers to control the execution flow, and adding Request and Response classes to track message ids and to buffer any incoming results that don't match the current request.

As a user of the API, you don't have to reason about when results come in. Your code will block until it starts seeing results that match your request message_id.

Although the gist I posted is a proof of concept, I think it maps pretty closely to the existing code. We would need to add additional state to the Net::LDAP::Connection class, but we could leave the existing public API intact. This also opens up the option for abandoning requests, though that would be in a separate PR.

For additional context, check out https://github.com/ruby-ldap/ruby-net-ldap/pull/133 and https://github.com/github/github-ldap/commit/620ce324bb82ca9109dfb5adc6bfa1b058328443

cc @mtodd @schaary

[jch]

Updated description pseudocode for more realistic failing case.

[mtodd]

@jch great breakdown of the issue, it helped me understand the problem well enough to build an alternative proposal as #138.

In #138, I tried to focus on queueing messages read from the socket that didn't map to the search being handled. l do this without introducing Fibers or extracting Request or Response objects mainly because I wanted to know if it could be done.

I agree that we should extract SearchRequest and SearchResponse objects, that we should have a better handle of execution flow (allowing for more asynchronous operation), and design around multi-threaded operations (because I think we can do this cleanly, with Fibers or otherwise). I think those should each be tackled separately so that we can measure the individual impact and consciously design and adjust the public API.

I'm a fan of changing the public API such that users work with the SearchResponse object, performing (blocking) reads on response.each et al. This is similar to how the Perl Net::LDAP API works, and likely similar to the Java LDAP implementation you referred me to before. It does afford more asynch workflows, the chance to abort operations, etc. But it is a change from the current API, though since we're pre-1.0, I wouldn't let that stop us.

Are we missing any alternative approaches that should be considered?

[mtodd]

@jch I think this can be closed since #138 was merged. Good stuff in this proposal that should be extracted, but not relevant to the original issue I think.

[jch]

:+1: