Skip to content

Fix: Use a dedicated sink for tar reading instead of a transform stream - #3646

Merged
BYK merged 1 commit into
masterfrom
kitchensink
Jun 16, 2017
Merged

BYK merged 1 commit into
masterfrom
kitchensink

Conversation

@BYK

@BYK BYK commented Jun 16, 2017 •

Copy link
Copy Markdown
Member

Summary

Potentially fixes #3011 (specifically https://git.io/vHAzW).

Turns out our ConcatStream implementation was only used to read
from tar-fs when creating a package to upload and put that stream
into a memory buffer. Since ConcatStream was implemented as a
stream.Transform and nothing was reading back from it, it had the
potential to just hang there until something reads from it. This
patch replaces that with a small script.

Test plan

Removed existing tests for the old module. Rely on other existing
tests for the replacement code.

**Summary**

Potentially fixes #3011 (specifically https://git.io/vHAzW)

Turns out our `ConcatStream` implementation was only used to read
from `tar-fs` when creating a package to upload and put that stream
into a memory buffer. Since `ConcatStream` was implemented as a
`stream.Transform` and nothing was reading back from it, it had the
potential to just hang there until something reads from it. This
patch replaces that with [a small script][1].

[1]: http://www.geekpeak.de/images/produkte/i22/22-go-away-or-i-will-replace-you-de.jpg

**Test plan**

Removed existing tests for the old module. Rely on other existing
tests for the replacement code.
@BYK
BYK requested review from bestander and zertosh June 16, 2017 00:56
@BYK BYK self-assigned this Jun 16, 2017

@bestander bestander left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Less code - good.
Merge once Circle is green

@tomsonpl

Copy link
Copy Markdown

@BYK hey, this issue is still happening. Do you have any plans to fix this?

i am receiving 9.154668655 Request "https://registry.npmjs.org/xxxxxx" finished with status code 200.

And gets stuck.

yarn v 1.22.10

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.

Publish without version prompt

3 participants