feat: Support for capping the number of origins in Agent

[JoshMock]

This relates to...

Addresses #3268, but only for Agent. Will address BalancedPool in a separate PR.

Rationale

Exposes an optional maxOrigins option on Agent to enforce a maximum number of origins that can have open connections. Throws AgentMaxOriginsReached error if attempting to dispatch a request to a new origin when maxOrigins has already been reached.

Changes

Tracks unique origins in Agent in a Set named kOrigins, alongside its internal kClients map, adding new origins during dispatch, and removing origins when the last connected client for an origin is disconnected. If dispatching the current request would exceed maxOrigins, throws AgentMaxOriginsReached.

Features

• Optional parameter maxOrigins added to Agent to enforce an origin limit

Bug Fixes

N/A

Breaking Changes and Deprecations

N/A

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[metcoder95]

Thanks for contributing!

For Agent the situation is a bit more complicated.
As it manages pool/client per origin, might be better to re-shape the approach for it by limiting the number of origins if desired (defaulting to Inifinity).

If no more room for adding more origins, requests should be dropped; there's no way an space will be available unless explicitly preempted.

[mcollina]

Good work! It's a good start! I wonder if it would be better to maintain a counter in the Agent instead of sum it up at every request.

[JoshMock]

For Agent the situation is a bit more complicated.
As it manages pool/client per origin, might be better to re-shape the approach for it by limiting the number of origins if desired (defaulting to Inifinity).

This approach makes sense to me, as controlling max connections would just become a downstream effect of limiting origins, with max connections effectively becoming origins * connections.

If no more room for adding more origins, requests should be dropped

When you say it's dropped, you mean that Agent shouldn't have a queue at all? If so, would it throw an exception like AgentMaxOriginsReached?

[JoshMock]

Okay, I've simplified it to be an origins restriction in Agent, which simplified things quite a bit. Agent[kOrigins] tracks unique origins in its own Set now.

I kept the queue in Agent until I get clarification from @metcoder95 on my question about whether to queue or throw an error. If we do keep the queue, should I switch it from an array to a FixedQueue, to align it more closely with PoolBase as you suggested here?

[metcoder95]

When you say it's dropped, you mean that Agent shouldn't have a queue at all? If so, would it throw an exception like AgentMaxOriginsReached?

Exactly; if a max limit of origins has been reached, I'm doubtful new connections to origins will be dropped in a timely manner (staling the request), as is complicated to ensure when a connection will be dropped to leave space for a new one.

[JoshMock]

@metcoder95 Agent now throws an error on dispatch when maxOrigins is reached, rather than managing its own queue. Please take a look!

[metcoder95]

lgtm already, just test for types is missed and should be good to go

[JoshMock]

Merged main in to account for the conflict introduced by #4425, and updated the description to use the PR template.

@metcoder95 this is ready to merge when you approve the latest merge commit.

[Uzlopak]

Add the Error suffix please.

Other than that codewise looks ok.

[Uzlopak]

I am actually now a little bit hesitant. Should we really call it AgentMaxOriginsReachedError? Or should it be just MaxOriginsReachedError?

@metcoder95
@mcollina

Could this be also be used in Pools or Clients? If so, we should remove the Prefix "Agent".

[Uzlopak]

@JoshMock

Dont change yet the code. Lets clarify first what the error is "doing", before you do extra work.

[JoshMock]

Lets clarify first what the error is "doing"

I'm not sure I understand what you mean. 🤔 Can you clarify?

I tend to agree with @Uzlopak that a more generic MaxOriginsReachedError is probably more useful, especially since it would be reusable if I add the same functionality to BalancedPool as I've planned. If we wanted to customize the error for each use case, the error message could be overridden rather than having a different Error class for each.

It's a quick change, so let me know if that's how we want to proceed.

[Uzlopak]

If you consider to use it in various places, than you can generalize it of course. Please proceed ;)

[JoshMock]

@Uzlopak updated in 5994344. 🙏

[JoshMock]

@Uzlopak @metcoder95 Any update on when we can merge this?

[metcoder95]

lgtm

[mcollina]

lgtm

[Uzlopak]

We changed the Error class thing with instanceof and symbols.

LGTM

[Uzlopak]

@metcoder95
@mcollina

can someone of you reapprove?

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.53%. Comparing base (13b2874) to head (b98b816).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4365   +/-   ##
=======================================
  Coverage   93.53%   93.53%           
=======================================
  Files         103      103           
  Lines       32349    32378   +29     
=======================================
+ Hits        30257    30286   +29     
  Misses       2092     2092           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[metcoder95]

Failures are unrelated to the PR itself