feat: Add callback context and redirect history 2 (nodejs#769)

* feat: Add callback context and redirect history.

* feat: Updated TypeScript types.

* fix: Make sure context exist before modifying it.

* fixup

* fixup

* fixup

* perf: reuse redirect handler

* fixup

* fixup

* fixuP

* fixup

* fixup

* Apply suggestions from code review

* fixup

Co-authored-by: Paolo Insogna <paolo@cowtech.it>

docs/api/Agent.md

@@ -19,7 +19,7 @@ Returns: `Agent`
 Extends: [`ClientOptions`](docs/api/Pool.md#parameter-pooloptions)
 
 * **factory** `(origin: URL, opts: Object) => Dispatcher` - Default: `(origin, opts) => new Pool(origin, opts)` 
-* **maxRedirections** `Integer` - Default: `0`.
+* **maxRedirections** `Integer` - Default: `0`. The number of HTTP redirection to follow unless otherwise specified in `DispatchOptions`.
 
 ## Instance Properties
 

docs/api/Dispatcher.md

@@ -201,7 +201,7 @@ Returns: `void`
 
 #### Parameter: `DispatchHandlers`
 
-* **onConnect** `(abort: () => void) => void` - Invoked before request is dispatched on socket. May be invoked multiple times when a request is retried when the request at the head of the pipeline fails.
+* **onConnect** `(abort: () => void, context: object) => void` - Invoked before request is dispatched on socket. May be invoked multiple times when a request is retried when the request at the head of the pipeline fails.
 * **onError** `(error: Error) => void` - Invoked when an error has occurred.
 * **onUpgrade** `(statusCode: number, headers: Buffer[] | null, socket: Duplex) => void` (optional) - Invoked when request is upgraded. Required if `DispatchOptions.upgrade` is defined or `DispatchOptions.method === 'CONNECT'`.
 * **onHeaders** `(statusCode: number, headers: Buffer[] | null, resume: () => void) => boolean` - Invoked when statusCode and headers have been received. May be invoked multiple times due to 1xx informational headers. Not required for `upgrade` requests.
@@ -327,6 +327,7 @@ Extends: [`RequestOptions`](#parameter-requestoptions)
 * **headers** `IncomingHttpHeaders`
 * **opaque** `unknown`
 * **body** `stream.Readable`
+* **context** `object`
 
 #### Example 1 - Pipeline Echo
 
@@ -419,6 +420,7 @@ The `RequestOptions.method` property should not be value `'CONNECT'`.
 * **trailers** `Record<string, string>` - This object starts out
   as empty and will be mutated to contain trailers after `body` has emitted `'end'`.
 * **opaque** `unknown`
+* **context** `object`
 
 #### Example 1 - Basic GET Request
 
@@ -567,6 +569,7 @@ Returns: `void | Promise<StreamData>` - Only returns a `Promise` if no `callback
 
 * **opaque** `unknown`
 * **trailers** `Record<string, string>`
+* **context** `object`
 
 #### Example 1 - Basic GET stream request
 

lib/agent.js

@@ -155,7 +155,7 @@ class Agent extends Dispatcher {
 
       const { maxRedirections = this[kMaxRedirections] } = opts
 
-      if (!Number.isInteger(maxRedirections) || maxRedirections < 0) {
+      if (maxRedirections != null && (!Number.isInteger(maxRedirections) || maxRedirections < 0)) {
         throw new InvalidArgumentError('maxRedirections must be a positive number')
       }
 

lib/api/api-connect.js

@@ -26,20 +26,21 @@ class ConnectHandler extends AsyncResource {
     addSignal(this, signal)
   }
 
-  onConnect (abort) {
+  onConnect (abort, context) {
     if (!this.callback) {
       throw new RequestAbortedError()
     }
 
     this.abort = abort
+    this.context = context
   }
 
   onHeaders () {
     throw new SocketError('bad connect')
   }
 
   onUpgrade (statusCode, headers, socket) {
-    const { callback, opaque } = this
+    const { callback, opaque, context } = this
 
     removeSignal(this)
 
@@ -48,7 +49,8 @@ class ConnectHandler extends AsyncResource {
       statusCode,
       headers: util.parseHeaders(headers),
       socket,
-      opaque
+      opaque,
+      context
     })
   }
 

lib/api/api-pipeline.js

@@ -84,6 +84,7 @@ class PipelineHandler extends AsyncResource {
     this.opaque = opaque || null
     this.handler = handler
     this.abort = null
+    this.context = null
 
     this.req = new PipelineRequest().on('error', util.nop)
 
@@ -137,7 +138,7 @@ class PipelineHandler extends AsyncResource {
     addSignal(this, signal)
   }
 
-  onConnect (abort) {
+  onConnect (abort, context) {
     const { ret, res } = this
 
     assert(!res, 'pipeline cannot be retried')
@@ -147,10 +148,11 @@ class PipelineHandler extends AsyncResource {
     }
 
     this.abort = abort
+    this.context = context
   }
 
   onHeaders (statusCode, headers, resume) {
-    const { opaque, handler } = this
+    const { opaque, handler, context } = this
 
     if (statusCode < 200) {
       return
@@ -165,7 +167,8 @@ class PipelineHandler extends AsyncResource {
         statusCode,
         headers: util.parseHeaders(headers),
         opaque,
-        body: this.res
+        body: this.res,
+        context
       })
     } catch (err) {
       this.res.on('error', util.nop)

lib/api/api-request.js

@@ -65,6 +65,7 @@ class RequestHandler extends AsyncResource {
     this.abort = null
     this.body = body
     this.trailers = {}
+    this.context = null
 
     if (util.isStream(body)) {
       body.on('error', (err) => {
@@ -75,16 +76,17 @@ class RequestHandler extends AsyncResource {
     addSignal(this, signal)
   }
 
-  onConnect (abort) {
+  onConnect (abort, context) {
     if (!this.callback) {
       throw new RequestAbortedError()
     }
 
     this.abort = abort
+    this.context = context
   }
 
   onHeaders (statusCode, headers, resume) {
-    const { callback, opaque, abort } = this
+    const { callback, opaque, abort, context } = this
 
     if (statusCode < 200) {
       return
@@ -100,7 +102,8 @@ class RequestHandler extends AsyncResource {
       headers: util.parseHeaders(headers),
       trailers: this.trailers,
       opaque,
-      body
+      body,
+      context
     })
   }
 

lib/api/api-stream.js

@@ -48,6 +48,7 @@ class StreamHandler extends AsyncResource {
     this.callback = callback
     this.res = null
     this.abort = null
+    this.context = null
     this.trailers = null
     this.body = body
 
@@ -60,16 +61,17 @@ class StreamHandler extends AsyncResource {
     addSignal(this, signal)
   }
 
-  onConnect (abort) {
+  onConnect (abort, context) {
     if (!this.callback) {
       throw new RequestAbortedError()
     }
 
     this.abort = abort
+    this.context = context
   }
 
   onHeaders (statusCode, headers, resume) {
-    const { factory, opaque } = this
+    const { factory, opaque, context } = this
 
     if (statusCode < 200) {
       return
@@ -79,7 +81,8 @@ class StreamHandler extends AsyncResource {
     const res = this.runInAsyncScope(factory, null, {
       statusCode,
       headers: util.parseHeaders(headers),
-      opaque
+      opaque,
+      context
     })
 
     if (

lib/api/api-upgrade.js

@@ -23,24 +23,26 @@ class UpgradeHandler extends AsyncResource {
     this.opaque = opaque || null
     this.callback = callback
     this.abort = null
+    this.context = null
 
     addSignal(this, signal)
   }
 
-  onConnect (abort) {
+  onConnect (abort, context) {
     if (!this.callback) {
       throw new RequestAbortedError()
     }
 
     this.abort = abort
+    this.context = null
   }
 
   onHeaders () {
     throw new SocketError('bad upgrade')
   }
 
   onUpgrade (statusCode, headers, socket) {
-    const { callback, opaque } = this
+    const { callback, opaque, context } = this
 
     assert.strictEqual(statusCode, 101)
 
@@ -50,7 +52,8 @@ class UpgradeHandler extends AsyncResource {
     this.runInAsyncScope(callback, null, null, {
       headers: util.parseHeaders(headers),
       socket,
-      opaque
+      opaque,
+      context
     })
   }
 

lib/core/request.js

@@ -128,7 +128,7 @@ class Request {
     assert(!this.aborted)
     assert(!this.completed)
 
-    return this[kHandler].onConnect(abort)
+    return this[kHandler].onConnect(abort, this.context)
   }
 
   onHeaders (statusCode, headers, resume) {

lib/handler/redirect.js

@@ -3,19 +3,24 @@
 const { InvalidArgumentError } = require('../core/errors')
 const util = require('../core/util')
 
+const redirectableStatusCodes = [300, 301, 302, 303, 307, 308]
+
 class RedirectHandler {
   constructor (agent, { maxRedirections, ...opts }, handler) {
     this.agent = agent
     this.location = null
     this.abort = null
     this.opts = opts
-    this.redirections = maxRedirections
+    this.maxRedirections = maxRedirections
     this.handler = handler
+    this.history = []
   }
 
-  onConnect (abort) {
+  onConnect (abort, context = {}) {
+    context.history = this.history
+
     this.abort = abort
-    this.handler.onConnect(abort)
+    this.handler.onConnect(abort, context)
   }
 
   onUpgrade (statusCode, headers, socket) {
@@ -27,17 +32,15 @@ class RedirectHandler {
   }
 
   onHeaders (statusCode, headers, resume) {
-    if ([300, 301, 302, 303, 307, 308].indexOf(statusCode) === -1) {
-      return this.handler.onHeaders(statusCode, headers, resume)
-    }
-
-    this.location = this.redirections === 0 ? null : parseLocation(headers)
+    this.location = this.history.length >= this.maxRedirections
+      ? null
+      : parseLocation(statusCode, headers)
 
     if (!this.location) {
       return this.handler.onHeaders(statusCode, headers, resume)
     }
 
-    this.redirections -= 1
+    this.history.push(new URL(this.opts.path, this.opts.origin).toString())
 
     const { origin, pathname, search } = util.parseURL(new URL(this.location, this.opts.origin))
     const path = search ? `${pathname}${search}` : pathname
@@ -102,7 +105,11 @@ class RedirectHandler {
   }
 }
 
-function parseLocation (headers) {
+function parseLocation (statusCode, headers) {
+  if (redirectableStatusCodes.indexOf(statusCode) === -1) {
+    return null
+  }
+
   for (let i = 0; i < headers.length; i += 2) {
     if (headers[i].length === 8 && headers[i].toString().toLowerCase() === 'location') {
       return headers[i + 1]