[http] add http connection pool idle

[klarose]

Description:
This adds the concept of a connection pool going "idle", implementing it for http1. A connection pool is idle if it has neither pending requests nor active requests. A user of a connection pool can ask it to
invoke a callback when the pool goes idle. A list of callbacks is maintained.

This is intended to be used to inform a mapping class when a connection pool is no longer used. The mapping class will use this information to help enforce a limit on the number of concurrent mapped connection pools. See #5337 (comment)

Risk Level: Low
Testing: Unit testing.
Docs Changes: None

[klarose]

My next pull request will implement this for http2. I've left it out of this PR so I can get feedback on the overall approach early, and to keep the PR small.

My plan for http2 is to use the following logic to decide whether the pool is idle:

   (primary || primary->numActiveRequests() == 0) &&
   (draining || draining->numActiveRequests() == 0) &&
   pending_requests.empty()

After that I'll implement the mapping class, then hook it in.

[lizan]

it might be overkill, did you consider using Common::CallbackManager?

[klarose]

Makes sense. I replaced idle_callbacks_ with that. I considered replacing drained_callbacks_, but it has some logic dependent on whether the list is empty, and that wasn't exposed by common::CallbackManager. Can probably be added in another PR.

[lizan]

LGTM modulo one nit.

[lizan]

nit: NOT_IMPLEMENTED_GCOVR_EXCL_LINE

[klarose]

This is actually being invoked by the base class when a pending request is cancelled. I had this in originally, but it failed because this line panicked in a UT.

[mattklein123]

A question to get started. Thank you.

[mattklein123]

Is it possible to merge checkForDrained() and checkForIdle() into a single function? I see there is some boolean logic below, but I think it should be possbile and would make the code less fragile. In general FAICT drained and idle are basically the same thing, but the intention of the callback is the same? Could we just use a single callback system and a different registration callback for your purpose and not have this new callback type at all?

[klarose]

Are you thinking of a callback object with something like onDrained vs onIdle?

Then, the checkForX function would be along the lines of:

if(drainedCondition):
 for each callback:
   callback.onDrained
if (idleCondition):
  for each callback:
   callback.onIdle

[mattklein123]

What I'm asking is whether this change is needed. AFAICT idle and drained are the same thing. Can't you just register for a drained callback in the place where you want to do something when it's idle?

[klarose]

My concern is that adding a drained callback has side effects -- namely, when the pool goes idle, it will actively close all of its upstream connections. My vision here is for the upstreams to stay active until we need to actually free up connection pools for other hash key values.

But, in the short term, it's probably not a big deal. I had planned on allocating and freeing them as necessary in the first iteration. But, now that I think about it, a better approach may be to only register the drained callback when there is pressure from the constraints.

My original thought was along these lines:

ConnectionPool* assignNewPool(hash) {
  pool = getActivePool(hash)
  if pool { return pool; }

  pool = getIdlePool();
  if (!pool) { pool = allocateNewPoolIfPossible(); }
  if (!pool) {
    // out of resources
    return nullptr;
  }

  pool->addIdleCallback([&]() {this->poolIdle(hash, pool); });
  return pool;
}

void poolIdle(hash, pool) {
  moveFromActiveToIdle(hash, pool);
}

I was worried that the act of registering a drained callback would be problematic. But, maybe it's not. Here's an alternative:

ConnectionPool* assignNewPool(hash) {
  pool = getActivePool(hash)
  if pool { return pool; }

  pool = getIdlePool();
  if (!pool) { pool = allocateNewPoolIfPossible(); }
  if (!pool) {
    for each activePool {
      activePool->addDrainedCallback([&](){this->poolIdle(hash, pool);});
    }
     // at this point, any pools which are currently idle will have invoked poolIdle.
     return getIdlePool();
  }

  return pool;
}

void poolIdle(hash, pool) {
  moveFromActiveToIdle(hash, pool);
}

I think I'll park this idle concept for now, and try what I've just suggested.

[klarose]

One thing I like about my "new" approach a bit better is that it could allow for some hysteresis on the draining. Start cleaning up pools if we cross a threshold, but before we run out, and stop cleaning them up when we cross below another. This would require that we add the ability to remove a drained callback, but we'll probably want that anyway.

[mattklein123]

I see what you are saying. I guess I would say two things:

1. I would probably start simple and just use the drained callback for now, and we can optimize later if needed.
2. if we do need to optimize, I think the implementation internally can actually share the same check idle/drained logic and just use a boolean to determine whether to close active connections or not. That would reduce the logic duplication.

WDYT?

[klarose]

Agreed. I'll close this.

[klarose]

As discussed above, we're going to try an approach that does not require this.