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

feat: allow connection header in request #1829

Merged
merged 21 commits into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
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
1 change: 1 addition & 0 deletions docs/api/Dispatcher.md
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo
* **origin** `string | URL`
* **path** `string`
* **method** `string`
* **reset** `boolean` (optional) - Default: `false` - Indicates whether the request should attempt to create a long-living connection by sending the `connection: keep-alive` header, or close it immediately after response by sending `connection: close`.
Copy link
Contributor

Choose a reason for hiding this comment

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

@metcoder95 This is phrased in a confusing way. Documentation implies that "true" means creating long-lived connection, while code seems to indicate the opposite.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, let me fix it opening a new PR, thanks for the heads up 🙇

* **body** `string | Buffer | Uint8Array | stream.Readable | Iterable | AsyncIterable | null` (optional) - Default: `null`
* **headers** `UndiciHeaders | string[]` (optional) - Default: `null`.
* **query** `Record<string, any> | null` (optional) - Default: `null` - Query string params to be embedded in the request URL. Note that both keys and values of query are encoded using `encodeURIComponent`. If for some reason you need to send them unencoded, embed query params into path directly instead.
Expand Down
7 changes: 5 additions & 2 deletions lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -1295,7 +1295,7 @@ function _resume (client, sync) {
}

function write (client, request) {
const { body, method, path, host, upgrade, headers, blocking } = request
const { body, method, path, host, upgrade, headers, blocking, reset } = request

// https://tools.ietf.org/html/rfc7231#section-4.3.1
// https://tools.ietf.org/html/rfc7231#section-4.3.2
Expand Down Expand Up @@ -1363,7 +1363,6 @@ function write (client, request) {

if (method === 'HEAD') {
// https://github.com/mcollina/undici/issues/258

// Close after a HEAD request to interop with misbehaving servers
// that may send a body in the response.

Expand All @@ -1377,6 +1376,10 @@ function write (client, request) {
socket[kReset] = true
}

if (reset) {
socket[kReset] = true
}

if (client[kMaxRequests] && socket[kCounter]++ >= client[kMaxRequests]) {
socket[kReset] = true
}
Expand Down
14 changes: 13 additions & 1 deletion lib/core/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ class Request {
upgrade,
headersTimeout,
bodyTimeout,
reset,
throwOnError
}, handler) {
if (typeof path !== 'string') {
Expand Down Expand Up @@ -97,6 +98,10 @@ class Request {
throw new InvalidArgumentError('invalid bodyTimeout')
}

if (reset != null && typeof reset !== 'boolean') {
throw new InvalidArgumentError('invalid reset')
}

this.headersTimeout = headersTimeout

this.bodyTimeout = bodyTimeout
Expand Down Expand Up @@ -139,6 +144,8 @@ class Request {

this.blocking = blocking == null ? false : blocking

this.reset = reset == null ? false : reset

this.host = null

this.contentLength = null
Expand Down Expand Up @@ -320,7 +327,12 @@ function processHeader (request, key, val) {
key.length === 10 &&
key.toLowerCase() === 'connection'
) {
throw new InvalidArgumentError('invalid connection header')
const value = val.toLowerCase()
if (value !== 'close' && value !== 'keep-alive') {
throw new InvalidArgumentError('invalid connection header')
} else if (value === 'close') {
request.reset = true
}
} else if (
key.length === 10 &&
key.toLowerCase() === 'keep-alive'
Expand Down
71 changes: 71 additions & 0 deletions test/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,77 @@ test('basic get', (t) => {
})
})

test('basic get with custom request.reset=true', (t) => {
t.plan(26)

const server = createServer((req, res) => {
t.equal('/', req.url)
t.equal('GET', req.method)
t.equal(`localhost:${server.address().port}`, req.headers.host)
t.equal(req.headers.connection, 'close')
t.equal(undefined, req.headers.foo)
t.equal('bar', req.headers.bar)
t.equal(undefined, req.headers['content-length'])
res.setHeader('Content-Type', 'text/plain')
res.end('hello')
})
t.teardown(server.close.bind(server))

const reqHeaders = {
foo: undefined,
bar: 'bar'
}

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

t.equal(client[kUrl].origin, `http://localhost:${server.address().port}`)

const signal = new EE()
client.request({
signal,
path: '/',
method: 'GET',
reset: true,
headers: reqHeaders
}, (err, data) => {
t.error(err)
const { statusCode, headers, body } = data
t.equal(statusCode, 200)
t.equal(signal.listenerCount('abort'), 1)
t.equal(headers['content-type'], 'text/plain')
const bufs = []
body.on('data', (buf) => {
bufs.push(buf)
})
body.on('end', () => {
t.equal(signal.listenerCount('abort'), 0)
t.equal('hello', Buffer.concat(bufs).toString('utf8'))
})
})
t.equal(signal.listenerCount('abort'), 1)

client.request({
path: '/',
reset: true,
method: 'GET',
headers: reqHeaders
}, (err, { statusCode, headers, body }) => {
t.error(err)
t.equal(statusCode, 200)
t.equal(headers['content-type'], 'text/plain')
const bufs = []
body.on('data', (buf) => {
bufs.push(buf)
})
body.on('end', () => {
t.equal('hello', Buffer.concat(bufs).toString('utf8'))
})
})
})
})

test('basic get with query params', (t) => {
t.plan(4)

Expand Down
12 changes: 1 addition & 11 deletions test/invalid-headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const { test } = require('tap')
const { Client, errors } = require('..')

test('invalid headers', (t) => {
t.plan(10)
t.plan(9)

const client = new Client('http://localhost:3000')
t.teardown(client.destroy.bind(client))
Expand Down Expand Up @@ -46,16 +46,6 @@ test('invalid headers', (t) => {
t.type(err, errors.InvalidArgumentError)
})

client.request({
path: '/',
method: 'GET',
headers: {
connection: 'close'
}
}, (err, data) => {
t.type(err, errors.InvalidArgumentError)
})

client.request({
path: '/',
method: 'GET',
Expand Down
68 changes: 68 additions & 0 deletions test/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -149,3 +149,71 @@ test('Absolute URL as pathname should be included in req.path', async (t) => {
t.equal(noSlashPath2Arg.statusCode, 200)
t.end()
})

test('DispatchOptions#reset', scope => {
scope.plan(3)

scope.test('Should throw if invalid reset option', t => {
t.plan(1)

t.rejects(request({
method: 'GET',
origin: 'http://somehost.xyz',
reset: 0
}), 'invalid reset')
})

scope.test('Should include "connection:close" if reset true', async t => {
const server = createServer((req, res) => {
t.equal('GET', req.method)
t.equal(`localhost:${server.address().port}`, req.headers.host)
t.equal(req.headers.connection, 'close')
res.statusCode = 200
res.end('hello')
})

t.plan(3)

t.teardown(server.close.bind(server))

await new Promise((resolve, reject) => {
server.listen(0, (err) => {
if (err != null) reject(err)
else resolve()
})
})

await request({
method: 'GET',
origin: `http://localhost:${server.address().port}`,
reset: true
})
})

scope.test('Should include "connection:keep-alive" if reset false', async t => {
const server = createServer((req, res) => {
t.equal('GET', req.method)
t.equal(`localhost:${server.address().port}`, req.headers.host)
t.equal(req.headers.connection, 'keep-alive')
res.statusCode = 200
res.end('hello')
})

t.plan(3)

t.teardown(server.close.bind(server))

await new Promise((resolve, reject) => {
server.listen(0, (err) => {
if (err != null) reject(err)
else resolve()
})
})

await request({
method: 'GET',
origin: `http://localhost:${server.address().port}`,
reset: false
})
})
})
2 changes: 1 addition & 1 deletion test/types/api.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Dispatcher, request, stream, pipeline, connect, upgrade } from '../..'
// request
expectAssignable<Promise<Dispatcher.ResponseData>>(request(''))
expectAssignable<Promise<Dispatcher.ResponseData>>(request('', { }))
expectAssignable<Promise<Dispatcher.ResponseData>>(request('', { method: 'GET' }))
expectAssignable<Promise<Dispatcher.ResponseData>>(request('', { method: 'GET', reset: false }))

// stream
expectAssignable<Promise<Dispatcher.StreamData>>(stream('', { method: 'GET' }, data => {
Expand Down
6 changes: 3 additions & 3 deletions test/types/dispatcher.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ expectAssignable<Dispatcher>(new Dispatcher())
expectAssignable<boolean>(dispatcher.dispatch({ origin: '', path: '', method: 'GET' }, {}))
expectAssignable<boolean>(dispatcher.dispatch({ origin: '', path: '', method: 'GET', headers: [] }, {}))
expectAssignable<boolean>(dispatcher.dispatch({ origin: '', path: '', method: 'GET', headers: {} }, {}))
expectAssignable<boolean>(dispatcher.dispatch({ origin: '', path: '', method: 'GET', headers: null }, {}))
expectAssignable<boolean>(dispatcher.dispatch({ origin: '', path: '', method: 'GET', headers: null, reset: true }, {}))
expectAssignable<boolean>(dispatcher.dispatch({ origin: new URL('http://localhost'), path: '', method: 'GET' }, {}))

// connect
Expand All @@ -30,7 +30,7 @@ expectAssignable<Dispatcher>(new Dispatcher())
expectAssignable<Promise<Dispatcher.ResponseData>>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0, query: { pageNum: 1, id: 'abc' } }))
expectAssignable<Promise<Dispatcher.ResponseData>>(dispatcher.request({ origin: '', path: '', method: 'GET', maxRedirections: 0, throwOnError: true }))
expectAssignable<Promise<Dispatcher.ResponseData>>(dispatcher.request({ origin: new URL('http://localhost'), path: '', method: 'GET' }))
expectAssignable<void>(dispatcher.request({ origin: '', path: '', method: 'GET' }, (err, data) => {
expectAssignable<void>(dispatcher.request({ origin: '', path: '', method: 'GET', reset: true }, (err, data) => {
expectAssignable<Error | null>(err)
expectAssignable<Dispatcher.ResponseData>(data)
}))
Expand Down Expand Up @@ -59,7 +59,7 @@ expectAssignable<Dispatcher>(new Dispatcher())
return new Writable()
}))
expectAssignable<void>(dispatcher.stream(
{ origin: '', path: '', method: 'GET' },
{ origin: '', path: '', method: 'GET', reset: false },
data => {
expectAssignable<Dispatcher.StreamFactoryData>(data)
return new Writable()
Expand Down
2 changes: 2 additions & 0 deletions types/dispatcher.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ declare namespace Dispatcher {
headersTimeout?: number | null;
/** The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use 0 to disable it entirely. Defaults to 30 seconds. */
bodyTimeout?: number | null;
/** Whether the request should stablish a keep-alive or not. Default `false` */
reset?: boolean;
/** Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server. Defaults to false */
throwOnError?: boolean;
}
Expand Down