Skip to content

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

Merged
titanism merged 1 commit into
forwardemail:masterfrom
slickmb:fix/emit_end_when_unzipping
Aug 15, 2023
Merged

fix: only emit 'end' after unzip is done writing#1766
titanism merged 1 commit into
forwardemail:masterfrom
slickmb:fix/emit_end_when_unzipping

Conversation

@slickmb

@slickmb slickmb commented May 3, 2023

Copy link
Copy Markdown
Contributor

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

  • I have ensured my pull request is not behind the main or master branch of the original repository.
  • I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • I have written a commit message that passes commitlint linting.
  • I have ensured that my code changes pass linting tests.
  • I have ensured that my code changes pass unit tests.
  • I have described my pull request and the reasons for code changes along with context if necessary.

@slickmb slickmb force-pushed the fix/emit_end_when_unzipping branch from 8792b3c to d61eb63 Compare May 3, 2023 22:27
@slickmb slickmb force-pushed the fix/emit_end_when_unzipping branch from d61eb63 to ef969fa Compare May 3, 2023 22:32
@titanism

titanism commented May 4, 2023

Copy link
Copy Markdown
Collaborator

is this ready for review?

@slickmb

slickmb commented May 4, 2023

Copy link
Copy Markdown
Contributor Author

yes, I believe it is.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

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

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

📣 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
@@            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 Coverage Δ
src/node/index.js 94.30% <100.00%> (+0.55%) ⬆️

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

@slickmb

slickmb commented May 8, 2023

Copy link
Copy Markdown
Contributor Author

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

@slickmb

slickmb commented May 31, 2023

Copy link
Copy Markdown
Contributor Author

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 titanism merged commit a29a062 into forwardemail:master Aug 15, 2023
@titanism

Copy link
Copy Markdown
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants