Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow passing request context to PoolOptions #3979

Open
blinsay opened this issue Dec 31, 2024 · 2 comments
Open

Allow passing request context to PoolOptions #3979

blinsay opened this issue Dec 31, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@blinsay
Copy link

blinsay commented Dec 31, 2024

This would solve...

When creating a new connection, I'd like to do a host lookup based on multiple parts of a request and create a new TLS connection. Sometimes the name of the cert is based on the origin in the URL, sometimes request headers need to be taken into account, and so on.

The implementation should look like...

PoolOptions.factory should take DispatchOptions as a third argument. It could pass the entire request in as a third argument here.

if (!dispatcher) {
dispatcher = this[kFactory](opts.origin, this[kOptions])
.on('drain', this[kOnDrain])
.on('connect', this[kOnConnect])
.on('disconnect', this[kOnDisconnect])
.on('connectionError', this[kOnConnectionError])
// This introduces a tiny memory leak, as dispatchers are never removed from the map.
// TODO(mcollina): remove te timer when the client/pool do not have any more
// active connections.
this[kClients].set(key, dispatcher)

This is a public API change that exposes a lot more data to PoolOptions.factory, so I didn't want to open a PR without checking with folks first. The diff to agent.js could be nice and small, but I'm not sure what else I'd have to do to make the change.

    if (!dispatcher) {
-     dispatcher = this[kFactory](opts.origin, this[kOptions])
+     dispatcher = this[kFactory](opts.origin, this[kOptions], opts)
        .on('drain', this[kOnDrain])
        .on('connect', this[kOnConnect])
        .on('disconnect', this[kOnDisconnect])
        .on('connectionError', this[kOnConnectionError])

      // This introduces a tiny memory leak, as dispatchers are never removed from the map.
      // TODO(mcollina): remove te timer when the client/pool do not have any more
      // active connections.
      this[kClients].set(key, dispatcher)
    }

I have also considered...

I've considered overriding Agent with a completely custom class to do this - it'd be a lot of work, but it would be nice to write less code and use the hook provided on PoolOptions.

Thanks!

@blinsay blinsay added the enhancement New feature or request label Dec 31, 2024
@blinsay blinsay changed the title Allow passing more request context to PoolOptions Allow passing request context to PoolOptions Dec 31, 2024
@mcollina
Copy link
Member

mcollina commented Jan 1, 2025

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@blinsay
Copy link
Author

blinsay commented Jan 2, 2025

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina sure! Before I get started, is this something you all would be interested in? Is the API change a concern?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants