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

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

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

slickmb
Copy link
Contributor

@slickmb slickmb commented May 3, 2023

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
Copy link
Collaborator

titanism commented May 4, 2023

is this ready for review?

@slickmb
Copy link
Contributor Author

slickmb commented May 4, 2023

yes, I believe it is.

@codecov-commenter
Copy link

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
Copy link
Contributor Author

slickmb commented May 8, 2023

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

@slickmb
Copy link
Contributor Author

slickmb commented May 31, 2023

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 ladjs:master Aug 15, 2023
@titanism
Copy link
Collaborator

v8.1.0 released which fixes this issue, thank you

npm install [email protected]

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