Skip to content

Support for handling compressed responses in the HTTP connector #138

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Support for handling compressed responses in the HTTP connector #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aec0333
This will add the capability for handling gzipped content to the
marcelog Sep 2, 2014
83b6400
adding check and test for deflate algo
marcelog Sep 4, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions src/lib/connectors/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var _ = require('../utils');
var qs = require('querystring');
var ForeverAgent = require('./_custom_agent');
var ConnectionAbstract = require('../connection');
var zlib = require('zlib');

/**
* Connector used to talk to an elasticsearch node via HTTP
Expand Down Expand Up @@ -123,8 +124,9 @@ HttpConnector.prototype.request = function (params, cb) {
var request;
var response;
var status = 0;
var headers;
var headers = {};
var log = this.log;
var buffers = [];

var reqParams = this.makeReqParams(params);

Expand All @@ -144,19 +146,32 @@ HttpConnector.prototype.request = function (params, cb) {
if (err) {
cb(err);
} else {
cb(err, response, status, headers);
response = Buffer.concat(buffers);
var zipHdr = headers['content-encoding'];
if (zipHdr && (zipHdr.match(/gzip/i) || zipHdr.match(/deflate/i))) {
zlib.unzip(response, function(gzErr, uncompressedResponse) {
Copy link
Contributor

@spalger spalger Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using zlib.unzip() instead of zlib.gunzip()?

I feel like there is a disconnect between the header you specified (Accept-Encoding: gzip, deflate) and the condition here. I think that there should be a check for deflate somewhere and that the zlib streams should be used instead of the async methods that wait for the end of the response.

Unfortunately, this is mostly just a guess (based on naming) because the node docs are pretty pitiful for those methods.

Copy link
Contributor Author

@marcelog marcelog Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spenceralger (replying to your comment about the use of unzip/gunzip that my commit wiped out, sorry):

I chose to use zlib.unzip() because it ends up creating an Unzip class. The nodejs doc for the Unzip class states that it will "Decompress either a Gzip- or Deflate-compressed stream by auto-detecting the header.". So I figured that would be the "safest bet". I agree on checking for the deflate algorithm, so I updated the code to include the check, and a unit test, so we test gzip and deflate in separate cases.

Copy link
Contributor

@spalger spalger Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

if(gzErr) {
err = gzErr;
response = response.toString('binary');
} else {
response = uncompressedResponse.toString('utf8');
}
cb(err, response, status, headers);
});
} else {
cb(err, response.toString('utf8'), status, headers);
}
}
}, this);

request = this.hand.request(reqParams, function (_incoming) {
incoming = _incoming;
status = incoming.statusCode;
headers = incoming.headers;
incoming.setEncoding('utf8');
response = '';

buffers = [];
incoming.on('data', function (d) {
response += d;
buffers.push(new Buffer(d));
Copy link
Contributor

@spalger spalger Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As recommended earlier, I think this would be better implemented using the zlib.createGunzip() and zlib.createInflate() methods to create streams that would eliminate this juggling of buffers.

Copy link
Contributor Author

@marcelog marcelog Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spenceralger Perhaps you're right, although my intention was to try to make as few changes to the code as possible, and the buffer juggling was already there, it was just hidden by the use of the '+' operator to concatenate the chunks of the response, that were assumed to be text and not binary data. Do you consider this is unacceptable as it is?

Copy link
Contributor

@spalger spalger Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't consider it a blocker. I can definitely make the change myself 😃

});

incoming.on('error', cleanUp);
Expand Down
74 changes: 74 additions & 0 deletions test/unit/specs/http_connector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ describe('Http Connector', function () {
var expectSubObject = require('../../utils/expect_sub_object');
var MockRequest = require('../../mocks/request');
var MockIncommingMessage = require('../../mocks/incomming_message');
var zlib = require('zlib');
var estr = require('event-stream');

nock.disableNetConnect();

Expand Down Expand Up @@ -302,6 +304,78 @@ describe('Http Connector', function () {
});
});

it('collects the whole request body (gzip compressed)', function (done) {
var server = nock('http://esjs.com:9200');
var con = new HttpConnection(new Host('http://esjs.com:9200'));
var elements = [];
for(var i = 0; i < 500; i++) {
elements.push({ "USER": "doc" });
}
var body = JSON.stringify(elements);
zlib.gzip(body, function(err, compressedBody) {
server
.get('/users/1')
.reply(200, compressedBody, {'Content-Encoding': 'gzip'});
Copy link
Contributor

@spalger spalger Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be Content-Encoding: deflate since you used zlib.deflate() to create the body?

Copy link
Contributor Author

@marcelog marcelog Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spenceralger Replying to 2 of your comments. I added a unit test, so we can test both gzip and deflate algorithms in separate cases (your comment was about setting content-encoding to gzip and using deflate to compress the data).

Your other comment was about testing it with a larger payload, so I created a small loop to send a lot of text, they are actually small json objects serialized into a string. Do you think it does the job?

Copy link
Contributor

@spalger spalger Sep 4, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should work great


con.request({
method: 'GET',
path: '/users/1'
}, function (err, resp, status) {
expect(err).to.be(undefined);
expect(resp).to.eql(body);
expect(status).to.eql(200);
server.done();
done();
});
});
});

it('collects the whole request body (deflate compressed)', function (done) {
var server = nock('http://esjs.com:9200');
var con = new HttpConnection(new Host('http://esjs.com:9200'));
var elements = [];
for(var i = 0; i < 500; i++) {
elements.push({ "USER": "doc" });
}
var body = JSON.stringify(elements);
zlib.deflate(body, function(err, compressedBody) {
server
.get('/users/1')
.reply(200, compressedBody, {'Content-Encoding': 'deflate'});

con.request({
method: 'GET',
path: '/users/1'
}, function (err, resp, status) {
expect(err).to.be(undefined);
expect(resp).to.eql(body);
expect(status).to.eql(200);
server.done();
done();
});
});
});

it('Can handle uncompress errors', function (done) {
var server = nock('http://esjs.com:9200');
var con = new HttpConnection(new Host('http://esjs.com:9200'));
var body = 'blah';
server
.get('/users/1')
.reply(200, body, {'Content-Encoding': 'gzip'});

con.request({
method: 'GET',
path: '/users/1'
}, function (err, resp, status) {
expect(err.errno).to.be(-3);
expect(resp).to.eql(body);
expect(status).to.eql(200);
server.done();
done();
});
});

it('Ignores serialization errors', function (done) {
var server = nock('http://esjs.com:9200');
var con = new HttpConnection(new Host('http://esjs.com:9200'));
Expand Down