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

Preserve `agents` across .defaults({...}) #156

Merged
merged 1 commit into from Apr 19, 2017
Merged

Preserve `agents` across .defaults({...}) #156

merged 1 commit into from Apr 19, 2017

Conversation

@jmonster
Copy link
Contributor

jmonster commented Jan 20, 2017

problem

const Wreck = require('wreck') and Wreck.defaults({}) instantiate their own agents rather than preserving the properties as you might expect when creating increasing granular http clients via .defaults()

(my) use case

declare application-level defaults via a hapijs plugin

const MAX_FREE_SOCKETS = process.env.MAX_FREE_SOCKETS || 256;
const MAX_SOCKETS = process.env.MAX_SOCKETS || 128;
const KEEP_ALIVE = process.env.KEEP_ALIVE || true;
const KEEP_ALIVE_MSECS = process.env.KEEP_ALIVE_MSECS || 512;

exports.register = function (server, options, next) {

    function configureAgent(agent /* , type */) {
        agent.maxFreeSockets = MAX_FREE_SOCKETS;
        agent.maxSockets = MAX_SOCKETS;
        agent.keepAlive = KEEP_ALIVE;
        agent.keepAliveMsecs = KEEP_ALIVE_MSECS;
    }

    configureAgent(require('wreck').agents.http);
    configureAgent(require('wreck').agents.https);
    configureAgent(require('wreck').agents.httpsAllowUnauthorized);
    configureAgent(require('http').globalAgent);
    configureAgent(require('https').globalAgent);

    next();
  }

exports.register.attributes = {
    name: 'configure-http-agents',
    version: '1'
};

create an http client pre-wired for our API

function generateHerokuClient() {

    const proto = options.proto || HEROKU_API_PROTO;
    const host = options.host || HEROKU_API_HOST;
    const timeout = options.timeout || HTTP_TIMEOUT;
    const userAgent = options['user-agent'] || '';
    const variant = options.variant || 'application/vnd.heroku+json; version=3';
    const headers = {
        'accept': variant,
        'user-agent': `${pkg.name}/${pkg.version} ${userAgent}`.trim()
    };

    const heroku = Wreck.defaults({
        baseUrl: `${proto}//${host}/`,
        headers,
        timeout,
        json: true
    });

    return heorku;
}

make succinct API calls

heroku.get('/version', (err, version) => {
    reply(version);
});
const wreck2 = wreck1.defaults({});

//using === because `.to.equal(Wreck.agents)` triggers a deepEqual explosion
expect(wreck1.agents === Wreck.agents).to.equal(true);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Jan 20, 2017

Contributor

Does expect(wreck1.agents).to.shallow.equal(Wreck.agents); work?

This comment has been minimized.

Copy link
@jmonster

jmonster Jan 20, 2017

Author Contributor

I didn't try that one but if this PR gets a 👍 otherwise I'd be glad to make the change

This comment has been minimized.

Copy link
@jmonster

jmonster Apr 3, 2017

Author Contributor

yes :) thanks!

@geek

This comment has been minimized.

Copy link
Member

geek commented Jan 20, 2017

Wreck supports passing a custom agent in the options (https://github.com/hapijs/wreck#advanced). Does this not work for your use case?

@jmonster

This comment has been minimized.

Copy link
Contributor Author

jmonster commented Jan 20, 2017

Well, before I answer that -- I intuitively expected the library to work the way this PR modifies it to. I was skeptical whether ya'll would agree with that but thought the PR would be the best way to frame the discussion. I only realized the library didn't work this way when I began triggering 502s by making too many concurrent requests --- this is avoided by limiting MAX_SOCKETS to a number the server in question can handle.

The issue I have with the agent option you suggest is that the library itself will no longer select an agent based on the proto (http/https) of the url being requested.

As a result, then I'm adding more boilerplate across the app to dynamically select that across my app, as I actually use .defaults() very heavily (I actually introduced this feature a few years ago). The agents object exists so that wreck can pick the appropriate agent based on the proto when the request is going out -- exactly the behavior I need/expected.

Thoughts?

@jmonster

This comment has been minimized.

Copy link
Contributor Author

jmonster commented Jan 24, 2017

Anything I can do to push this across the finish line?

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Mar 21, 2017

@geek ping.

httpsAllowUnauthorized: new Https.Agent({ maxSockets: Infinity, rejectUnauthorized: false })
};
// ensure defaults() defaults to `Wreck.agents.{http|https|httpsAllowUnauthorized}`
if (defaults && defaults.agents) {

This comment has been minimized.

Copy link
@geek

geek Mar 21, 2017

Member

We either need to assert that when agents exists that it has all of the necessary agents created (http, https, httpsAllowUnauthorized) or that we merge these existing defaults with the agents object.

expect(wreck2.agents === Wreck.agents).to.equal(true);

done();
});

This comment has been minimized.

Copy link
@geek

geek Mar 21, 2017

Member

Please add a few more tests for setting one or some of the available agents

@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 21, 2017

@jmonster sorry for the delay, the review didn't get submitted :/

@geek

This comment has been minimized.

Copy link
Member

geek commented Mar 24, 2017

@jmonster we will also want an update to the readme

@geek geek added the feature label Mar 24, 2017
@geek geek self-assigned this Mar 24, 2017
@jmonster

This comment has been minimized.

Copy link
Contributor Author

jmonster commented Mar 25, 2017

👍 I'll come back to this when I get a chance, hopefully in the next week

@jmonster jmonster force-pushed the jmonster:agents branch 2 times, most recently from e46b330 to 67df588 Apr 4, 2017
Copy link
Contributor Author

jmonster left a comment

thanks @geek @cjihrig; please let me know if/what needs more attention :)

@@ -320,7 +337,7 @@ internals.redirectMethod = function (code, method, options) {

internals.Client.prototype.read = function (res, options, callback) {

options = Hoek.applyToDefaultsWithShallow(options || {}, this._defaults, internals.shallowOptions);
options = Hoek.applyToDefaultsWithShallow(this._defaults, options || {}, internals.shallowOptions);

This comment has been minimized.

Copy link
@jmonster

jmonster Apr 4, 2017

Author Contributor

I was confused/surprised when I saw this! Please confirm this is correct? I believe without this change we're giving higher precedence to .defaults than the options passed into read itself

This comment has been minimized.

Copy link
@geek

geek Apr 17, 2017

Member

Your change looks correct. I'll double-check before publishing

https: new Https.Agent({ maxSockets: Infinity }),
http: new Http.Agent({ maxSockets: Infinity }),
httpsAllowUnauthorized: new Https.Agent({ maxSockets: Infinity, rejectUnauthorized: false })
};

This comment has been minimized.

Copy link
@jmonster

jmonster Apr 4, 2017

Author Contributor

I'm very open to suggestions to better articulate/organize these changes

@@ -2506,9 +2506,6 @@ describe('Defaults', () => {
const wreckB = Wreck.defaults(optionsB);
const wreckAB = wreckA.defaults(optionsB);

// const agent = new Http.Agent();
// expect(Object.keys(agent.sockets).length).to.equal(0);

This comment has been minimized.

Copy link
@jmonster

jmonster Apr 4, 2017

Author Contributor

wasn't sure if these comments were left intentionally

@jmonster jmonster force-pushed the jmonster:agents branch 2 times, most recently from ad3fae9 to 72ff157 Apr 4, 2017
@jmonster jmonster force-pushed the jmonster:agents branch from 72ff157 to c9d3300 Apr 4, 2017
@geek geek added this to the 12.1.0 milestone Apr 17, 2017
@geek geek merged commit 25f4f9f into hapijs:master Apr 19, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jmonster

This comment has been minimized.

Copy link
Contributor Author

jmonster commented Apr 20, 2017

awesome thanks so much for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.