ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.58k stars 1.33k forks source link

fix: only emit 'end' after unzip is done writing #1766

Closed slickmb closed 1 year ago

slickmb commented 1 year ago

When using node:stream.pipeline or node:stream/promises.pipeline with superagent, the pipeline often fails with a 'write after end' error or with truncated data. This is because, in _pipeContinue, we are emitting 'end' as soon as the response is done. This is fine for normal requests, but for compressed content, there is an extra transform stream introduced in the pipe line. If we emit end on the agent request before the unzipObject transform stream is done writing data, pipeline will end the target stream too early and unzipObject will attempt to continue writing to the closed stream (resulting in an error, or, depending on the stream being written to, truncated data).

To fix, we make sure to emit 'end' on the agent request only after the unzipObject stream has finished writing.

Checklist

titanism commented 1 year ago

is this ready for review?

slickmb commented 1 year ago

yes, I believe it is.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.25 :tada:

Comparison is base (73c7efb) 94.19% compared to head (ef969fa) 94.45%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1766 +/- ## ========================================== + Coverage 94.19% 94.45% +0.25% ========================================== Files 14 14 Lines 1155 1155 ========================================== + Hits 1088 1091 +3 + Misses 67 64 -3 ``` | [Impacted Files](https://app.codecov.io/gh/ladjs/superagent/pull/1766?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ladjs) | Coverage Δ | | |---|---|---| | [src/node/index.js](https://app.codecov.io/gh/ladjs/superagent/pull/1766?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ladjs#diff-c3JjL25vZGUvaW5kZXguanM=) | `94.30% <100.00%> (+0.55%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

slickmb commented 1 year ago

If there's anything I can do to help with the review of this code, please let me know.

slickmb commented 1 year ago

Please let me know if there's anything I can do to help facilitate the review of this code. This bug can result in data loss for anyone using this library to pipe GZIP'ed content to a stream. The bug seems more prevalent when running node 18 or higher.

titanism commented 1 year ago

v8.1.0 released which fixes this issue, thank you

npm install superagent@8.1.0

release notes @ https://github.com/ladjs/superagent/releases/tag/v8.1.0