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: ensuring close event is emited after stream has ended. #332

Merged
merged 1 commit into from
Oct 27, 2022

Conversation

webark
Copy link
Contributor

@webark webark commented Oct 6, 2022

For node 18 compatibility

Fixes #321

Node 18 pipelines specifically require the end event to be emitted and handled before the close event is.

I tried a test with a local tar file and it was still passing without the fix, but a remote tar file failed, so just stuck to that.

@webark webark requested a review from a team as a code owner October 6, 2022 00:33
lib/parse.js Outdated Show resolved Hide resolved
@webark
Copy link
Contributor Author

webark commented Oct 6, 2022

@jeferson-sb @lukekarrys let me know if you have any thoughts on this.

@webark webark force-pushed the fix/properly-emitting-close-event branch from dd46841 to 096dc4d Compare October 13, 2022 06:10
@webark
Copy link
Contributor Author

webark commented Oct 13, 2022

@lukekarrys squashed the commits and reworded to pass the commit lint.

@webark
Copy link
Contributor Author

webark commented Oct 20, 2022

@lukekarrys pinging again to see if you can approve the verifications.

@lukekarrys
Copy link
Contributor

Thanks for sticking with this @webark! I left a note above about a comment, but I decided I'd rather approve and merge and not risk forgetting about this (yet again).

Thanks again! This should be released as a patch within the next week.

@github-actions github-actions bot mentioned this pull request Oct 27, 2022
@webark webark deleted the fix/properly-emitting-close-event branch October 27, 2022 01:48
@webark
Copy link
Contributor Author

webark commented Oct 27, 2022

Thank you! :) Just saw your previous comment and was about to push a change. I'll check back in a couple of months once node 18 has baked a bit and look into opening a PR to switch it to nextTick

@bergundy
Copy link

nit: Promise.resolve().then which works on Node 10 is equivalent to queueMicrotask and cheaper than nextTick.
It's not critical, just thought I'd bring this to your attention.
Thanks everyone for putting this fix together and reviewing!

@webark
Copy link
Contributor Author

webark commented Oct 27, 2022

@bergundy I didn't look into the performance case of that :( Should I open another PR with the Promise approach?

@bergundy
Copy link

It’s really not too critical imho, up to you.

@bergundy
Copy link

Here’s more information about the difference between tasks and microtasks FTR: https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/

@webark
Copy link
Contributor Author

webark commented Oct 27, 2022

That's was a good read! thanks for sharing 😊

@lukekarrys Do you know how long that node 10 is going to be supported?

We could also leverage the promise conditionally depending on if the micro task is available if node 10 support is going to be around for longer than a couple of months.

lukekarrys added a commit that referenced this pull request Oct 27, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
lukekarrys added a commit that referenced this pull request Oct 27, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
@bergundy
Copy link

I see a release PR was created by a bot: #326, can we get this approved please?

lukekarrys added a commit that referenced this pull request Oct 29, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
lukekarrys added a commit that referenced this pull request Oct 29, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
lukekarrys added a commit that referenced this pull request Oct 29, 2022
I was able to replicate the same behavior from #332 by piping a readable
stream with a `highWaterMark` of 1 to extract. I confirmed this test
still failed without the fixes from #332 and now passes.
@lukekarrys
Copy link
Contributor

I see a release PR was created by a bot: #326, can we get this approved please?

This has been released as 6.1.12. Sorry for the delay, I wanted to do a few upstream tests in npm/cli first. Thanks to @webark for the fix and @bergundy for the review and help!

ksibisamir added a commit to SaTT-Wallet/Backend that referenced this pull request May 26, 2023
<h3>Snyk has created this PR to upgrade tar from 6.1.11 to 6.1.14.</h3>

:information_source: Keep your dependencies up-to-date. This makes it
easier to fix existing vulnerabilities and to more quickly identify and
fix newly disclosed vulnerabilities when they affect your project.
<hr/>

- The recommended version is **3 versions** ahead of your current
version.
- The recommended version was released **22 days ago**, on 2023-05-02.


<details>
<summary><b>Release notes</b></summary>
<br/>
  <details>
    <summary>Package name: <b>tar</b></summary>
    <ul>
      <li>
<b>6.1.14</b> - <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases/tag/v6.1.14">2023-05-02</a></br><p>6.1.14</p>
      </li>
      <li>
<b>6.1.13</b> - <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases/tag/v6.1.13">2022-12-07</a></br><h2><a
href="https://snyk.io/redirect/github/npm/node-tar/compare/v6.1.12...v6.1.13">6.1.13</a>
(2022-12-07)</h2>
<h3>Dependencies</h3>
<ul>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/cc4e0ddfe523a0bce383846a67442c637a65d486"><code>cc4e0dd</code></a>
<a href="https://snyk.io/redirect/github/npm/node-tar/pull/343"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/343/hovercard">#343</a> bump
minipass from 3.3.6 to 4.0.0</li>
</ul>
      </li>
      <li>
<b>6.1.12</b> - <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases/tag/v6.1.12">2022-11-01</a></br><h2><a
href="https://snyk.io/redirect/github/npm/node-tar/compare/v6.1.11...v6.1.12">6.1.12</a>
(2022-10-31)</h2>
<h3>Bug Fixes</h3>
<ul>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/57493ee66ece50d62114e02914282fc37be3a91a"><code>57493ee</code></a>
<a href="https://snyk.io/redirect/github/npm/node-tar/pull/332"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/332/hovercard">#332</a>
ensuring close event is emited after stream has ended (<a
class="user-mention notranslate" data-hovercard-type="user"
data-hovercard-url="/users/webark/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/webark">@ webark</a>)</li>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/b003c64f624332e24e19b30dc011069bb6708680"><code>b003c64</code></a>
<a href="https://snyk.io/redirect/github/npm/node-tar/pull/314"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/314/hovercard">#314</a>
replace deprecated String.prototype.substr() (<a class="issue-link
js-issue-link" data-error-text="Failed to load title"
data-id="1192619366" data-permission-text="Title is private"
data-url="isaacs/node-tar#314"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/314/hovercard"
href="https://snyk.io/redirect/github/isaacs/node-tar/pull/314">#314</a>)
(<a class="user-mention notranslate" data-hovercard-type="user"
data-hovercard-url="/users/CommanderRoot/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/CommanderRoot">@
CommanderRoot</a>, <a class="user-mention notranslate"
data-hovercard-type="user"
data-hovercard-url="/users/lukekarrys/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/lukekarrys">@ lukekarrys</a>)</li>
</ul>
<h3>Documentation</h3>
<ul>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/f12992932f171ea248b27fad95e7d489a56d31ed"><code>f129929</code></a>
<a href="https://snyk.io/redirect/github/npm/node-tar/pull/313"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/313/hovercard">#313</a> remove
dead link to benchmarks (<a class="issue-link js-issue-link"
data-error-text="Failed to load title" data-id="1175996338"
data-permission-text="Title is private"
data-url="isaacs/node-tar#313"
data-hovercard-type="pull_request"
data-hovercard-url="/isaacs/node-tar/pull/313/hovercard"
href="https://snyk.io/redirect/github/isaacs/node-tar/pull/313">#313</a>)
(<a class="user-mention notranslate" data-hovercard-type="user"
data-hovercard-url="/users/yetzt/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/yetzt">@ yetzt</a>)</li>
<li><a
href="https://snyk.io/redirect/github/npm/node-tar/commit/c1faa9f44001dfb0bc7638b2850eb6058bd56a4a"><code>c1faa9f</code></a>
add examples/explanation of using tar.t (<a class="user-mention
notranslate" data-hovercard-type="user"
data-hovercard-url="/users/isaacs/hovercard"
data-octo-click="hovercard-link-click"
data-octo-dimensions="link_type:self"
href="https://snyk.io/redirect/github/isaacs">@ isaacs</a>)</li>
</ul>
      </li>
      <li>
<b>6.1.11</b> - <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases/tag/v6.1.11">2021-08-26</a></br><p>6.1.11</p>
      </li>
    </ul>
from <a
href="https://snyk.io/redirect/github/isaacs/node-tar/releases">tar
GitHub release notes</a>
  </details>
</details>


<details>
  <summary><b>Commit messages</b></summary>
  </br>
  <details>
    <summary>Package name: <b>tar</b></summary>
    <ul>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/4aaffc862f4e991f7965ecf6527072c4423ecb49">4aaffc8</a>
6.1.14</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/4cbdd674bfb2bda2769b94410650fd803e2f55ef">4cbdd67</a>
deps: minipass@5.0.0</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/75d3081ccf91853e13b7e0e28a077347b5a1fe3e">75d3081</a>
fix: update repository url in package.json</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/82bb3286a299903465a941bd70252843cf308a1e">82bb328</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/5f316363790f925d01a5809718b2958d0f3c0661">5f31636</a>
chore: bump @ npmcli/template-oss from 4.10.0 to 4.11.0</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/a044a87c6c7fb3ace4ea9bf903c63f0f15965398">a044a87</a>
chore: release 6.1.13 (#344)</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/cc4e0ddfe523a0bce383846a67442c637a65d486">cc4e0dd</a>
deps: bump minipass from 3.3.6 to 4.0.0</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/5dcfcb37fd5f7189be7ce63ef85ae3fbbc47da89">5dcfcb3</a>
chore: bump events-to-array from 1.1.2 to 2.0.3</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/329caed7d218f1784592f98380ff5a76968141ec">329caed</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/72f6e3915a80ee0b4c6e759412b1c460f156f62c">72f6e39</a>
chore: bump @ npmcli/template-oss from 4.8.0 to 4.10.0</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/001eafbfe77b10aa41c06081d7d3c9a3a7913240">001eafb</a>
chore: release 6.1.12</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/ac1026a69f9e0f5043a3f52c6f49c42b43b2066a">ac1026a</a>
chore: dry up template-oss config</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/2e45b112bdb6e88d32fa09b3eab2482637493b6c">2e45b11</a>
chore: use a local instead of remote file for test</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/79378ef9d044d0e992582f5a4768d90e4e2c1e3b">79378ef</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/eaea26d7d8dbd5b2c8236b64df0f56ae5704cf2b">eaea26d</a>
chore: bump @ npmcli/template-oss from 4.7.1 to 4.8.0</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/57493ee66ece50d62114e02914282fc37be3a91a">57493ee</a>
fix: ensuring close event is emited after stream has ended</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/1e3fadfedf9ea35da5dc7d70926a6864de6381cc">1e3fadf</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/24045dcefb1febc0d201566598ec9a378abe372d">24045dc</a>
chore: bump @ npmcli/template-oss from 4.6.2 to 4.7.1</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/b003c64f624332e24e19b30dc011069bb6708680">b003c64</a>
fix: replace deprecated String.prototype.substr() (#314)</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/d9edb344a1c799abfb5bc82e0f134865911160ab">d9edb34</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/c78c108abd93c4cd88373f7f6733f7c04f7f8a7f">c78c108</a>
chore: bump @ npmcli/template-oss from 4.6.1 to 4.6.2</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/2a49e7aaedc4138c6b94104bdaf8de56d86d7876">2a49e7a</a>
chore: postinstall for dependabot template-oss PR</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/39c60adaed54340c746c7ca68ff9e209ebe62110">39c60ad</a>
chore: bump @ npmcli/template-oss from 4.5.1 to 4.6.1</li>
<li><a
href="https://snyk.io/redirect/github/isaacs/node-tar/commit/08cc1562bd1a80394f41eaf5c1c11d92176f8446">08cc156</a>
chore: bump @ npmcli/eslint-config from 3.1.0 to 4.0.0</li>
    </ul>

<a
href="https://snyk.io/redirect/github/isaacs/node-tar/compare/e573aeea19d4d650908b7f6bf0a1ad8dce9f1736...4aaffc862f4e991f7965ecf6527072c4423ecb49">Compare</a>
  </details>
</details>
<hr/>

**Note:** *You are seeing this because you or someone else with access
to this repository has authorized Snyk to open upgrade PRs.*

For more information: <img
src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiIyMjI0ZTc5Mi00MGE2LTQxYWMtOWYxNS02NTFhZGIyOWVhNjkiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6IjIyMjRlNzkyLTQwYTYtNDFhYy05ZjE1LTY1MWFkYjI5ZWE2OSJ9fQ=="
width="0" height="0"/>

🧐 [View latest project
report](https://app.snyk.io/org/satt/project/b89486be-ad07-4d6c-a51a-2fa8a25baa00?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🛠 [Adjust upgrade PR
settings](https://app.snyk.io/org/satt/project/b89486be-ad07-4d6c-a51a-2fa8a25baa00/settings/integration?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🔕 [Ignore this dependency or unsubscribe from future upgrade
PRs](https://app.snyk.io/org/satt/project/b89486be-ad07-4d6c-a51a-2fa8a25baa00/settings/integration?pkg&#x3D;tar&amp;utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr#auto-dep-upgrades)

<!---
(snyk:metadata:{"prId":"2224e792-40a6-41ac-9f15-651adb29ea69","prPublicId":"2224e792-40a6-41ac-9f15-651adb29ea69","dependencies":[{"name":"tar","from":"6.1.11","to":"6.1.14"}],"packageManager":"npm","type":"auto","projectUrl":"https://app.snyk.io/org/satt/project/b89486be-ad07-4d6c-a51a-2fa8a25baa00?utm_source=github&utm_medium=referral&page=upgrade-pr","projectPublicId":"b89486be-ad07-4d6c-a51a-2fa8a25baa00","env":"prod","prType":"upgrade","vulns":[],"issuesToFix":[],"upgrade":[],"upgradeInfo":{"versionsDiff":3,"publishedDate":"2023-05-02T22:46:07.312Z"},"templateVariants":[],"hasFixes":false,"isMajorUpgrade":false,"isBreakingChange":false,"priorityScoreList":[]})
--->
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.

[BUG] Pipeline stream unexpectly being closed when using extract
4 participants