Skip to content

Commit

Permalink
fix(client): skip body if method is head
Browse files Browse the repository at this point in the history
In case of HEAD method `client.parser.[HTTPParser.kOnHeadersComplete]`
should tell the http parser to skip the body by returning 1 or true.
Also `this._lastBody` assignment is skipped.

See https://github.com/creationix/http-parser-js/blob/master/http-parser.js#L362
  • Loading branch information
dnlup committed Jan 17, 2020
1 parent bef4d56 commit 204c09e
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 4 deletions.
15 changes: 11 additions & 4 deletions lib/client.js
Expand Up @@ -217,10 +217,15 @@ class Client extends EventEmitter {
// TODO move this[kCallbacks] from being an array. The array allocation
// is showing up in the flamegraph.
const cb = this[kCallbacks].shift()
const request = this[kRequests].shift()
let skipBody
this._needHeaders--
this._lastBody = new Readable({ read: this[kRead].bind(this) })
this._lastBody.push = this[kRequests].shift().wrapSimple(this._lastBody, this._lastBody.push)

if (request.method === 'HEAD') {
skipBody = 1
} else {
this._lastBody = new Readable({ read: this[kRead].bind(this) })
this._lastBody.push = request.wrapSimple(this._lastBody, this._lastBody.push)
}
cb(null, {
statusCode,
headers: parseHeaders(headers),
Expand All @@ -230,6 +235,8 @@ class Client extends EventEmitter {
if (this.closed && this[kQueue].length() === 0) {
this.destroy()
}

return skipBody
}

this.parser[HTTPParser.kOnBody] = (chunk, offset, length) => {
Expand All @@ -239,7 +246,7 @@ class Client extends EventEmitter {
this.parser[HTTPParser.kOnMessageComplete] = () => {
const body = this._lastBody
this._lastBody = null
body.push(null)
body && body.push(null)
}

this[kReadCb] = () => {
Expand Down
129 changes: 129 additions & 0 deletions test/client.js
Expand Up @@ -36,6 +36,31 @@ test('basic get', (t) => {
})
})

test('basic head', (t) => {
t.plan(7)

const server = createServer((req, res) => {
t.strictEqual('/', req.url)
t.strictEqual('HEAD', req.method)
t.strictEqual('localhost', req.headers.host)
res.setHeader('content-type', 'text/plain')
res.end('hello')
})
t.tearDown(server.close.bind(server))

server.listen(0, () => {
const client = new Client(`http://localhost:${server.address().port}`)
t.tearDown(client.close.bind(client))

client.request({ path: '/', method: 'HEAD' }, (err, { statusCode, headers, body }) => {
t.error(err)
t.strictEqual(statusCode, 200)
t.strictEqual(headers['content-type'], 'text/plain')
t.strictEqual(body, null)
})
})
})

test('get with host header', (t) => {
t.plan(7)

Expand Down Expand Up @@ -67,6 +92,31 @@ test('get with host header', (t) => {
})
})

test('head with host header', (t) => {
t.plan(7)

const server = createServer((req, res) => {
t.strictEqual('/', req.url)
t.strictEqual('HEAD', req.method)
t.strictEqual('example.com', req.headers.host)
res.setHeader('content-type', 'text/plain')
res.end('hello from ' + req.headers.host)
})
t.tearDown(server.close.bind(server))

server.listen(0, () => {
const client = new Client(`http://localhost:${server.address().port}`)
t.tearDown(client.close.bind(client))

client.request({ path: '/', method: 'HEAD', headers: { host: 'example.com' } }, (err, { statusCode, headers, body }) => {
t.error(err)
t.strictEqual(statusCode, 200)
t.strictEqual(headers['content-type'], 'text/plain')
t.strictEqual(body, null)
})
})
})

function postServer (t, expected) {
return function (req, res) {
t.strictEqual(req.url, '/')
Expand Down Expand Up @@ -290,6 +340,33 @@ test('10 times GET', (t) => {
})
})

test('10 times HEAD', (t) => {
const num = 10
t.plan(3 * 10)

const server = createServer((req, res) => {
res.end(req.url)
})
t.tearDown(server.close.bind(server))

server.listen(0, () => {
const client = new Client(`http://localhost:${server.address().port}`)
t.tearDown(client.close.bind(client))

for (var i = 0; i < num; i++) {
makeRequest(i)
}

function makeRequest (i) {
client.request({ path: '/' + i, method: 'HEAD' }, (err, { statusCode, headers, body }) => {
t.error(err)
t.strictEqual(statusCode, 200)
t.strictEqual(body, null)
})
}
})
})

test('20 times GET with pipelining 10', (t) => {
const num = 20
t.plan(3 * num + 1)
Expand Down Expand Up @@ -348,6 +425,58 @@ function makeRequestAndExpectUrl (client, i, t, cb) {
})
}

test('20 times HEAD with pipelining 10', (t) => {
const num = 20
t.plan(3 * num + 1)

let count = 0
let countGreaterThanOne = false
const server = createServer((req, res) => {
count++
setTimeout(function () {
countGreaterThanOne = countGreaterThanOne || count > 1
res.end(req.url)
}, 10)
})
t.tearDown(server.close.bind(server))

// needed to check for a warning on the maxListeners on the socket
process.on('warning', t.fail)
t.tearDown(() => {
process.removeListener('warning', t.fail)
})

server.listen(0, () => {
const client = new Client(`http://localhost:${server.address().port}`, {
pipelining: 10
})
t.tearDown(client.close.bind(client))

for (let i = 0; i < num; i++) {
makeRequest(i)
}

function makeRequest (i) {
makeHeadRequestAndExpectUrl(client, i, t, () => {
count--

if (i === num - 1) {
t.ok(countGreaterThanOne, 'seen more than one parallel request')
}
})
}
})
})

function makeHeadRequestAndExpectUrl (client, i, t, cb) {
return client.request({ path: '/' + i, method: 'HEAD' }, (err, { statusCode, headers, body }) => {
cb()
t.error(err)
t.strictEqual(statusCode, 200)
t.strictEqual(body, null)
})
}

test('A client should enqueue as much as twice its pipelining factor', (t) => {
const num = 10
let sent = 0
Expand Down

0 comments on commit 204c09e

Please sign in to comment.