http.request() support socket pooling configuration? about q-io HOT 9 CLOSED

From what I can tell it uses the global agent because it does not specify the agent attribute: https://github.com/kriskowal/q-io/blob/master/http.js#L278

I'm fine with this, but my guess is others may want to control the agent on a request by request basis, so this issue could probably turn into a feature request.

from q-io.

in a similar vane, a knox client provides a similar interface to node's http client but adds an additional layer of logic to make it easier to work with amazon's s3. i've been wrapping that client to use q-io/reader and q-io/writer on the requests and responses and i'd like to see something within q-io/http that would take care of that for me. it's essentially the logic that already exists in q-io/http.request to map the _request interaction to something based on promises - i just want to have the chance to use something other than node's http (or https) as the source of that request.

EDIT: in case it isn't clear, the reason i said that my request is in a similar vane is because i expect that a utility to turn an instance of node's http.ClientRequest into something based on promises would satisfy @doapp-ryanp's needs too. maybe something along the lines of this API

var req = require('q-io/http').ClientRequest(require('http').request(options));

from q-io.

sorry... one more thought - i guess a more complete API would expose q-io/http.IncomingMessage as well.

from q-io.

Q-IO/HTTP request now accepts an "agent" property on the "request". Does that address this socket pooling issue adequately @doapp-ryanp?

@neonstalwart, Can you link a reference or describe what you mean by IncomingMessage? Is this distinct from http.ClientResponse or http.ServerResponse?

Please reopen attn me. Trying to wrangle my task queue.

from q-io.

@kriskowal yep that does the trick. Thanks!

from q-io.

@kriskowal it's been a while since i had my head in the code that caused me to make that comment so i'm kind of fuzzy on what i had in mind but i believe what i was suggesting was a utility for turning http://nodejs.org/api/http.html#http_http_incomingmessage into something based on promises and it's quite possible that q-io/http.ClientResponse is actually already this but now i don't recall if i just didn't see it previously or if there was some reason i thought a distinct IncomingMessage would be useful.

however, the "issue" i was trying to have addressed is that q-io/http.request is hardcoded to use node's http (or https) module as the way to generate _request. in addition to that, the mapping of the options passed to node's http.request are also hard-coded (which was the original issue raised here).

if you were to refactor q-io/http.request so that it generated _request (as it does now) but then used some utilities that were exported from q-io/http to make the request and response promise-based then we would have the building blocks to be able to use another client with the same API as node's http.request to generate a request - knox is one example of such a client.

var req = require('q-io/http').ClientRequest(knoxClient.request(options));
Q.when(req, function (response) {
    // response would be promise-based stream
    response.forEach( ... );
});

if you took on this idea, you would have something like the following in q-io/http (consider this code just to be an outline of the idea).

exports.request = function (request) {
    return Q.when(request, function (request) {
        request = exports.normalizeRequest(request);
        var ssl = request.ssl;
        var http = ssl ? HTTPS :  HTTP;
        var headers = request.headers || {};
        headers.host = headers.host || request.host;

        var _request = exports.ClientRequest(http.request({
            host: request.host,
            port: request.port || (ssl ? 443 : 80),
            path: request.path || "/",
            method: request.method || "GET",
            headers: headers,
            agent: request.agent
        }), request.charset);

        return Q.when(request.body.forEach(_request.write), _request.close).then(function () {
            return _request;
        });
    });
};

exports.ClientRequest = function (_request, charset) {
    return Q.when(_request, function (_request) {
        var deferred = Q.defer();

        _request.on('response', function (_response) {
            deferred.resolve(exports.ClientResponse(_response, request.charset));
            _response.on('error', deferred.reject); // ?! this is going to reject an already resolved deferred
        });
        _request.on('error', deferred.reject);
        return deferred.promise;
    });
};

i've probably messed up the logic in that code but hopefully it's good enough to give an outline of what i mean.

as far as how any of this relates to the original issue, code that was previously using q-io/http.request when it didn't pass along the agent could have been changed like so

// didn't work
var request = require('q-io/http').request;
var req = request({
    host: ' ... ',
    port: ' ... ',
    agent: anAgent
});

// while waiting on q-io to add support for agent could be refactored to
var ClientRequest = require('q-io/http').ClientRequest;
var req = ClientRequest(require('http').request({
    host: ' ... ',
    port: ' ... ',
    agent: anAgent
}));

from q-io.

Ah, thanks @neonstalwart. I think you’ll find that all mostly works, and you can use the NodeReader(nodeReadable) and NodeWriter(nodeWritable) constructors to create Q-IO streams from Node.js streams. These are in q-io/reader and q-io/writer, but will be in q-io/node/reader and q-io/node/writer in v2 unless something changes.

from q-io.

yes, those are what i used and it worked except for one small thing that didn't work as advertised - writer is missing the node property that exists in reader and is also documented as existing so i had to add it myself.

var writer = new Writer(stream);
return writer.then(function (writer) {
    // XXX: This is missing from the q-io writer even though the docs talk about it
    writer.node = stream;
    return writer;
});

from q-io.

@neonstalwart Thanks, I’ve added writer.node in my working copy. Sorry for missing that.

from q-io.