Allow RetryHandler to stop without throwing

[mikaelkaron]

At the moment onHeaders takes throwOnError into account like this:

if (this.throwOnError && statusCode >= 400) {
  this.runInAsyncScope(getResolveErrorBodyCallback, null,
    { callback, body, contentType, statusCode, statusMessage, headers }
  )
} else {
  this.runInAsyncScope(callback, null, null, {
    statusCode,
    headers,
    trailers: this.trailers,
    opaque,
    body,
    context
  })
}

However, onError always passes an error:

this.runInAsyncScope(callback, null, err, { opaque })

Would it make sense to have onError work similar to onHeaders when calling callback:

onError (err) {
  const { res, callback, body, trailers, context, opaque } = this

  removeSignal(this)

  if (callback) {
    // TODO: Does this need queueMicrotask?
    this.callback = null
    queueMicrotask(() => {
      if (this.throwOnError) {
        this.runInAsyncScope(callback, null, err, { opaque })
      }
      else {
        const { statusCode, headers } = err
        this.runInAsyncScope(callback, null, null, {
          statusCode,
          headers,
          trailers,
          opaque,
          body,
          context
        })
      }
    })
  }

  if (res) {
    this.res = null
    // Ensure all queued handlers are invoked before destroying res.
    queueMicrotask(() => {
      util.destroy(res, err)
    })
  }

  if (body) {
    this.body = null
    util.destroy(body, err)
  }
}

[mikaelkaron]

Another version of onError could be:

onError (err) {
  const { res, callback, body, opaque } = this

  removeSignal(this)

  if (callback) {
    // TODO: Does this need queueMicrotask?
    this.callback = null
    queueMicrotask(() => {
      if (this.throwOnError) {
        this.runInAsyncScope(callback, null, err, { opaque })
      }
      else {
        this.runInAsyncScope(callback, null, null, {
         ...err,
          opaque
        })
      }
    })
  }

  if (res) {
    this.res = null
    // Ensure all queued handlers are invoked before destroying res.
    queueMicrotask(() => {
      util.destroy(res, err)
    })
  }

  if (body) {
    this.body = null
    util.destroy(body, err)
  }
}

[mikaelkaron]

My particular use-case is that I'm using a RetryHandler but I don't want to have my request rejected if the default onRetry fails.

[mikaelkaron]

Yeah. I guess now the problem is more along “how do I make a RetryHandler not error (reject) when all retries are exhausted”

[metcoder95]

Maybe some sort of extra callback that can be added when creating the handler that receives the full response in exchange, but that will remove the magic from the handler

[mikaelkaron]

I've been looking at the code and I can't see any obvious ways of telling it to "stop retry and just pass data" in a simple way (actually, the whole retry continues until callback is passed with an error).

A workaround could be to call retryHandler.handler.onHeaders from inside the retry method (my own code), but it would be nice if RetryHandler had something that would allow me to just "continue"

perhaps the signature of callback (onRetry) would be onRetry(err, data) and if !err the just pass on data?

[metcoder95]

That's doable; what puts me over the fence is the goal of the RetryHandler itself and what we are trying to do.
Overall the RetryHandler was designed to handle automatic retries on errors (either network or responses) but adding the possibility to to bypass that automatic retry and forward an error as a simple response seems counterintuitive, but let's give it a try, I might be wrong here.

[mikaelkaron]

I see it more like “how do you want to handle failure” where it’s ok to reject with a detailed error, but it would be just as ok to pass the error downstream (or even modify it).

same function, more options (as both of them stop retries).