-
-
Notifications
You must be signed in to change notification settings - Fork 675
Decompression Interceptor #4317
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job!
I'd just suggest to implement the decompressor in accordance to RFC-9110, in that way decompressor can work with different combination in according to standards.
I believe the only missing part might be the support for multiple encodings on a response
Co-authored-by: Carlos Fuentes <[email protected]>
|
@metcoder95 Thanks! All the above have been implemented along with documentation. Let me know if anything else is required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! I left few commenets.
Do not forget about documentation and TS types
lib/interceptor/decompress.js
Outdated
| controller.resume() | ||
| }) | ||
|
|
||
| pipeline(this.#decompressors, (err) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pipeline returns a stream which you can subscribe for the chunks of data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just tests seems failing
|
Failures were due to lack of If everything else looks good, I'd like to get out a final commit to clean up some code |
This comment has been minimized.
This comment has been minimized.
|
@Uzlopak these optimizations are sound and the docs are a nice touch! I noticed annotations are missing on onResponseData, and onResponseStart, onResponseEnd and onResponeError dont have return types on their annotations but lgtm otherwise |
|
I mean, can you integrate them into this PR please? I dont know the types for those methods, and did not want to dig deeper. LOL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied my recommended changes.
It adds the said performance optimizations.
Also I added jsdoc which resulted to basically understand the full PR and despite the before existing performance issues, the code is as far as I can see correct.
@metcoder95
can you please review my changes and approve and merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After talk with Matteo, need some experimental warning or use different approach, readable stream instead of data-event
|
We can start with an experimental warning and take it from there |
|
Exactly. Can you take care of it? |
|
@Uzlopak I looked at Do you think its worth making changes to the jsdocs and code at this stage? Everything still functions as expected |
|
We did not merge yet, and if you can make the correct jsdoc, then please add them. Also we need to add an experimentalwarning OR you need to change it to a readable stream. |
|
Sounds good, I updated the jsdocs (some code as well) and added the experimental flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small not added for documentation, rest lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with a nit.
lib/interceptor/decompress.js
Outdated
| compress: createInflate, | ||
| 'x-compress': createInflate, | ||
| ...(createZstdDecompress ? { zstd: createZstdDecompress } : {}) | ||
| })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to use a normal object instead, this is just initialization overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for documentation purposes:
After a chat with Matteo I understand the issue, he sees with this PR.
While the interceptor is working, the majority of its code is about managing the streams. All that logic on when to pause and when to resume is a potential beesnest for errors.
Unfortunately there is no good example in the project on how to do it properly.
Other interceptors have the handler logic extracted in the handlers folder. This one is an amalgam of interceptor and handler.
I am personally not well versed in these parts of the project. I really wished it was somehow more intuitive or atleast there was a handler and interceptor which was the gold standard for handlers and interceptors. Then we could use that as a blueprint to standardize handlers and/or interceptors.
E.g. i had a look at the dump interceptor and it seemed that it is more like we need it. But it is storing the payload into the memory. Not the logic on how we can stream the payload out.
IIRC The cache interceptor seems to be better, but it is too complicated to grok it in short time.
Anyhow. There was for sure some thought when interceptors and handlers were designed, but it is not easy to grok.
So we have onResponseStart, onResponseData and onResponseEnd. So they are like events from a classical stream. But there is nothing intuitive in it, that I could just code a simple pipelining of the incoming response?!
I guess for all the other parts than the body of http requests/response the Interceptors are good?! But for handling the body it feels clunky.
Anyhow… i will make this week a deep dive into the interceptor stuff.
Yeah, but this is aside of the interceptors, is more about the Dispatch handler. They are intertwined in some way, but the Request events are the ones that manages the data flow, not necessarily the interceptors (the interceptors are just abstractions on top of the Dispatcher handlers). I can see the potential issue with the body pipelining into the decompressors (same problem I saw when I was playing at the beginning when the issue was made). Everything boils down to the body processing with the decompressor streams, it might be a bit noisy and potentially have some issues, but cannot think on a better way (right now). I can revisit and adjust later on and based on issues, but not see another way currently. |
|
Happy to revisit and improve upon this as well. In the meantime, it would be great if we could merge this |
This relates to...
Address proposal brought up here
and is related to #4316
Rationale
This PR implements a decompression interceptor for Undici, as discussed in the linked proposal and issue. The goal is to provide automatic response decompression for
request()(matching the behavior offetch()), reducing boilerplate and improving developer experience when working with compressed HTTP responses.Changes
content-encodingandcontent-lengthheaders when decompressingFeatures
Status