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: dump interceptor #3118

Merged
merged 34 commits into from May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
6d975ea
feat: add dump interceptor
metcoder95 Apr 14, 2024
5024ae1
fix(types): interceptor definitions
metcoder95 Apr 14, 2024
befe478
Merge branch 'main' into feat/dump_interceptor
metcoder95 Apr 14, 2024
da70cbd
docs: update maxSize description
metcoder95 Apr 16, 2024
62a4cde
Merge branch 'main' into feat/dump_interceptor
metcoder95 Apr 16, 2024
d82b51b
refactor: leftover
metcoder95 Apr 16, 2024
33b7126
docs: adjust
metcoder95 Apr 16, 2024
e3190f1
feat: abort on dumped
metcoder95 Apr 17, 2024
2fd173d
fix: return on header
metcoder95 Apr 23, 2024
85af461
refactor: apply suggestions
metcoder95 Apr 25, 2024
f5cc824
feat: extend from decorator handler
metcoder95 Apr 25, 2024
675c261
Merge branch 'main' into feat/dump_interceptor
metcoder95 Apr 25, 2024
c43c80c
fix: missing handler
metcoder95 Apr 25, 2024
8548c8e
feat: add dumpOnAbort
metcoder95 Apr 25, 2024
84c0d8b
Merge branch 'main' into feat/dump_interceptor
metcoder95 Apr 25, 2024
13ba7e0
Merge branch 'main' into feat/dump_interceptor
metcoder95 Apr 26, 2024
bf75a5b
test: adjust test
metcoder95 Apr 26, 2024
8607411
fix: bad consumer
metcoder95 Apr 28, 2024
428a953
Merge branch 'main' into feat/dump_interceptor
metcoder95 Apr 28, 2024
8fae866
refactor: tweaks
metcoder95 May 1, 2024
4492ab7
Merge branch 'main' into feat/dump_interceptor
metcoder95 May 8, 2024
cc42e2d
test: simplify
metcoder95 May 8, 2024
c60ee90
test: disable on windows
metcoder95 May 9, 2024
38b24fb
Merge branch 'main' into feat/dump_interceptor
metcoder95 May 10, 2024
fb4ccd8
refactoor Apply suggestions from code review
metcoder95 May 10, 2024
0d67095
chore: dump on abort by default
metcoder95 May 12, 2024
e60add7
fix: typo
metcoder95 May 13, 2024
3a68023
fix: test
metcoder95 May 13, 2024
3e3295f
refactor: Apply suggestions from code review
metcoder95 May 13, 2024
2265a19
fix: lint
metcoder95 May 13, 2024
ae685cf
fix: refactor
metcoder95 May 13, 2024
85ab3d6
Merge branch 'main' into feat/dump_interceptor
metcoder95 May 13, 2024
fb5af55
fix: cleanup leftovers
metcoder95 May 15, 2024
ab67b1b
Merge branch 'main' into feat/dump_interceptor
metcoder95 May 15, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 38 additions & 0 deletions docs/docs/api/Dispatcher.md
Expand Up @@ -952,6 +952,44 @@ const client = new Client("http://example.com").compose(
);
```

##### `dump`
mcollina marked this conversation as resolved.
Show resolved Hide resolved

The `dump` interceptor enables you to dump the response body from a request upon a given limit.

**Options**
- `maxSize` - The maximum size (in bytes) of the response body to dump. If the size of the request's body exceeds this value then the connection will be closed. Default: `1048576`.
- `abortOnDumped` - States whether or not abort the request after the response's body being dumped. Default: `true`.
- `waitForTrailers` - Hints the dispatcher to wait for trailers if the response's body has been dumped. Default: `false`.
ronag marked this conversation as resolved.
Show resolved Hide resolved
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved

> The `Dispatcher#options` also gets extended with the options `dumpMaxSize`, `abortOnDumped`, and `waitForTrailers` which can be used to configure the interceptor at a request-per-request basis.

**Example - Basic Dump Interceptor**

```js
const { Client, interceptors } = require("undici");
const { dump } = interceptors;

const client = new Client("http://example.com").compose(
dump({
maxSize: 1024,
abortOnDumped: true,
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
waitForTrailers: false,
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
})
);

// or
client.dispatch(
{
path: "/",
method: "GET",
dumpMaxSize: 1024,
abortOnDumped: true,
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
waitForTrailers: false,
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
},
handler
);
```

## Instance Events

### Event: `'connect'`
Expand Down
3 changes: 2 additions & 1 deletion index.js
Expand Up @@ -40,7 +40,8 @@ module.exports.RedirectHandler = RedirectHandler
module.exports.createRedirectInterceptor = createRedirectInterceptor
module.exports.interceptors = {
redirect: require('./lib/interceptor/redirect'),
retry: require('./lib/interceptor/retry')
retry: require('./lib/interceptor/retry'),
dump: require('./lib/interceptor/dump')
}

module.exports.buildConnector = buildConnector
Expand Down
129 changes: 129 additions & 0 deletions lib/interceptor/dump.js
@@ -0,0 +1,129 @@
'use strict'

const util = require('../core/util')
const { InvalidArgumentError, RequestAbortedError } = require('../core/errors')
const DecoratorHandler = require('../handler/decorator-handler')

class DumpHandler extends DecoratorHandler {
#maxSize = 1024 * 1024
#abort = null
#dumpOnAbort = true
#dumped = false
#aborted = false
#size = 0
#reason = null
#handler = null

constructor (
{ maxSize, dumpOnAbort },
handler
) {
super(handler)

if (maxSize != null && (!Number.isFinite(maxSize) || maxSize < 1)) {
throw new InvalidArgumentError('maxSize must be a number greater than 0')
}

if (dumpOnAbort != null && typeof dumpOnAbort !== 'boolean') {
throw new InvalidArgumentError('dumpOnAbort must be a boolean')
}

this.#maxSize = maxSize ?? this.#maxSize
this.#dumpOnAbort = dumpOnAbort ?? this.#dumpOnAbort
Copy link
Member

Choose a reason for hiding this comment

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

How about not having this as an option and just have it always enabled? We might also want to have a dump timeout in addition to a maxSize?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this goes in line with your suggestions for the PR.
Don't have strong opinion on this one, I believe there might be situations where you don't want to automatically dump on abort, but if we add it, it might never get used.

So let's simplify and follow your suggestions, if we see use cases or issues we can always add it 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding the timeout, shouldn't that already be covered by the bodyTimeout?
It might look redundant, and to have effect it might require to always set it lower to the bodyTimeout set for the Client/Agent

this.#handler = handler
}

onConnect (abort) {
if (this.#aborted) {
abort(this.#reason)
return
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.#aborted) {
abort(this.#reason)
return
}
if (this.#aborted) {
abort(this.#reason)
return
}

This can't happen?


this.#abort = abort
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved

this.#handler.onConnect(
this.#dumpOnAbort === true ? this.#customAbort.bind(this) : this.#abort
)
}

#customAbort (reason) {
this.#aborted = true
this.#reason = reason
}

// TODO: will require adjustment after new hooks are out
onHeaders (statusCode, rawHeaders, resume, statusMessage) {
ronag marked this conversation as resolved.
Show resolved Hide resolved
const headers = util.parseHeaders(rawHeaders)
const contentLength = headers['content-length']

if (contentLength != null && contentLength > this.#maxSize) {
throw new RequestAbortedError(
`Response size (${contentLength}) larger than maxSize (${
this.#maxSize
})`
)
}

return this.#handler.onHeaders(
statusCode,
rawHeaders,
resume,
statusMessage
)
}

onError (err) {
if (
!(err instanceof RequestAbortedError) &&
(!this.#dumped || this.#aborted)
) {
this.#handler.onError(err)
return
}

if (this.#dumped) {
this.#handler.onComplete([])
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
}
}

onData (chunk) {
ronag marked this conversation as resolved.
Show resolved Hide resolved
this.#size = this.#size + chunk.length

if (this.#size >= this.#maxSize) {
this.#dumped = true
this.#handler.onData('')
Copy link
Member

Choose a reason for hiding this comment

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

This is strange?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise the request keeps hanging, tried a workaround without luck; find it quite counterintuitive but most likely is something that might need to be addressed at the request level, but not sure the scope of this PR

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to address this before landing. Do you have minimal a repro? I can take a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

nvm, I found the issue; it was on me, the request was in the aborted state and couldn't continue because already aborted but I never called the onError hook from the original handler. That made the work

this.#handler.onComplete([])
}

return true
}

onComplete (trailers) {
if (!this.#dumped) {
this.#handler.onComplete(trailers)
}
}
}

function createDumpInterceptor (
{ maxSize: defaultMaxSize, dumpOnAbort: defaultDumpOnAbort } = {
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved
maxSize: 1024 * 1024,
dumpOnAbort: true
}
) {
return dispatch => {
return function Intercept (opts, handler) {
const { dumpMaxSize = defaultMaxSize, dumpOnAbort = defaultDumpOnAbort } =
opts

const dumpHandler = new DumpHandler(
{ maxSize: dumpMaxSize, dumpOnAbort },
handler
)
metcoder95 marked this conversation as resolved.
Show resolved Hide resolved

return dispatch(opts, dumpHandler)
}
}
}

module.exports = createDumpInterceptor