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

buffer chunk after flushing buffer in write if paused #28

Closed
wants to merge 2 commits into from

Conversation

everett1992
Copy link

4c5a106 handled a convoluted case where there is a chunk in the buffer
AND we're in a flowing state during a write call which caused out of
order writes.

The fix was to flush the buffer before emitting the new chunk, but it
didn't account for destinations pausing the stream after flushing part
of the buffer. This caused issues in npm / pacote / npm-registry-fetch.

That specific issue is demonstrated in everett1992/make-fetch-happen-tar-extract-error
and occurs when make-fetch-happen res.body is piped to a tar.extract
stream.

Fixes #27, npm/cli#3884, npm/make-fetch-happen#63

Caleb ツ Everett added 2 commits December 1, 2021 14:32
4c5a106 handled a convoluted case where there is a chunk in the buffer
AND  we're in a flowing state during a write call which caused out of
order writes.

The fix was to flush the buffer before emitting the new chunk, but it
didn't account for destinations pausing the stream after flushing part
of the buffer. This caused issues in npm/pacote/npm-registry-fetch.

That specific issue is demonstrated in everett1992/make-fetch-happen-tar-extract-error
and occurs when make-fetch-happen res.body is piped to a tar.extract
stream.

Fixes isaacs#27 npm/cli#3884 make-fetch-happen#63
@nlf
Copy link

nlf commented Dec 2, 2021

this is a most excellent fix! i'm totally impressed with how far you went to chase this one down, and your patch here is undoubtedly the correct behavior. thank you for your hard work on this one!

@everett1992
Copy link
Author

everett1992 commented Dec 6, 2021

Anything I need to do to ship this? And after it's shipped how do we update npm's dependencies and include that npm release in node?

@isaacs isaacs closed this in 2a0f468 Dec 6, 2021
@isaacs
Copy link
Owner

isaacs commented Dec 6, 2021

Thanks! Great catch!

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.

write can emit out of order chunks on a partial flush
3 participants