Skip to content

http: introduce getAllHeaders() #772

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

http: introduce getAllHeaders() #772

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
28a8002
http: introduce `getAllHeaders()`
iankronquist Dec 16, 2014
7d76ff1
test: replace faulty test-http-header-get-all
Fishrock123 Feb 9, 2015
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
9 changes: 9 additions & 0 deletions doc/api/http.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,15 @@ Example:

var contentType = response.getHeader('content-type');

### response.getAllHeaders()

Reads out all headers that are already been queued but not yet sent to the
Copy link
Contributor

Choose a reason for hiding this comment

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

that are should be that have

client. This can only be called before headers get implicitly flushed.

Example:

var headers = response.getAllHeaders();

### response.removeHeader(name)

Removes a header that's queued for implicit sending.
Expand Down
8 changes: 8 additions & 0 deletions lib/_http_outgoing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,14 @@ OutgoingMessage.prototype.getHeader = function(name) {
};


OutgoingMessage.prototype.getAllHeaders = function() {
if (!this._headers)
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we returning undefined instead of an empty object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I think it was just copied from getHeader().

I had taken it because I thought it was done so that the api was closer to getHeader(). I think an object makes more sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Returning undefined allows us differentiate "lack of _headers set" (i.e., possibly flushed) from "empty headers set."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds reasonable. I'll keep it.

Copy link

Choose a reason for hiding this comment

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

Does this mean that I always should check a type of the returning value? Are there cases when you need to check that at least one header was set?
I think that it would be better to have separate method to check this. Outgoing message can be immediately initialized with _headers = {} and it will allow to do not check every setHeader() call that the property was initialized

Copy link

Choose a reason for hiding this comment

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

Right now the method returns one of two types: Object with headers or undefined. And second one does not solve the problem of caller. I want all the headers, but it said "no headers yet", but it can just return an empty set.

The method have dual meaning, now it is hard to override this with saving exisiting semantics. Also this behaviour here are forcing a caller to be more complex, because it needed to handle unexpected behaviour

Example

res.getAllHeaderNames = function () {
    var headers = this.getAllHeaders();
    if (!headers) {
         // 1. Why should I do this check? 
         // 2. Can I return [] or i should inherit the behaviour? 
         return undefined;
    }
    return Object.keys(headers);
};

res.getContentHeaderNames = function () {
    var names = this.getAllHeaderNames();
    if (!names) {
         // again! It haunting me!
         return undefined;
    }
    return names.filter(...);
};

Also inspected https://github.com/iojs/io.js/blob/v1.x/lib/_http_outgoing.js : there are tons of _headers is not null checks. Immediately initializing _headers as {} will allow to avoid them all and possible improve the performance of http

else
return util._extend({}, this._headers);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it might be worth using Object.create(null) instead of {} to avoid extraneous properties on the object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, why not.

Copy link
Member

Choose a reason for hiding this comment

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

Object.create(null) is 15-30x slower than an object literal, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. We should use whatever _headers uses, I think. (So the result would be 100% identical)

Copy link
Contributor

Choose a reason for hiding this comment

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

The performance hit is unfortunate. Anyone know if v8 is working on making that better?

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, getAllHeaders should never be called in a hot loop, so optimizing should take a backseat to correctness – I don't want the user to be able to use the returned object to modify the outgoing headers, since that would be some spooky action at a distance. util._extend is ideal for this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrisdickinson Of course.

The discussion is weather to extend {} or Object.create(null). :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The performance hit is unfortunate. Anyone know if v8 is working on making that better?

No, V8 really wants you to use Map for these kind of use cases, and "use strong" even makes it impossible to abuse objects as maps.

};


OutgoingMessage.prototype.removeHeader = function(name) {
if (arguments.length < 1) {
throw new Error('`name` is required for removeHeader(name).');
Expand Down
31 changes: 31 additions & 0 deletions test/parallel/test-http-header-get-all.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
var common = require('../common');
var assert = require('assert');
var http = require('http');

var s = http.createServer(function(req, res) {
var contentType = 'content-type';
var plain = 'text/plain';
res.setHeader(contentType, plain);
assert.ok(!res.headersSent);
res.writeHead(200);
assert.ok(res.headersSent);
res.end('hello world\n');
// This checks that after the headers have been sent, getHeader works
// and does not throw an exception (joyent/node Issue 752)
assert.doesNotThrow(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the assert.doesNotThrow() or function are needed.

function() {
assert.deepStrictEqual({ 'content-type': 'text/plain' }, res.getAllHeaders());
}
);
});

s.listen(common.PORT, runTest);

function runTest() {
http.get({ port: common.PORT }, function(response) {
response.on('end', function() {
s.close();
});
response.resume();
});
}